Fix Optional Amount Serialization #552

JeremyRubin · 2021-01-14T18:51:01Z

This patch fixes the Optional Amount serialization for Amount types and adds tests that ensure that round tripping actually works.

This issue comes up because e.g., {amt: null} and {} are theoretically equivalent types, but our code did not properly handle the {amt: null} serialization output by serde_json.

thomaseizinger

utack da21294

Made some minor suggestions, feel to deal with them however you see fit :)

thomaseizinger · 2021-01-14T23:23:01Z

src/util/amount.rs

+    #[cfg(feature = "serde")]
+    #[test]
+    fn serde_as_sat_opt() {
+        use serde_json;


There is serde_test in case you don't want to rely on JSON-specific semantics when testing your implementations.

serde_test for example makes it quite easy to test Visitor implementations against various scenarios like owned vs borrowed data.

We are currently fixing some of this retroactively in rust-secp: rust-bitcoin/rust-secp256k1#270

src/util/amount.rs

JeremyRubin · 2021-01-14T23:45:47Z

W.r.t. the changes suggested by @thomaseizinger to the newly introduced test; my preference is to not modify the test code as it is copy/pasted from the earlier test for the bitcoin representation with minimal changes. If someone wants to rewrite both tests that seems like a separate PR.

dr-orlovsky

I have suggested some minor test improvements which I'd like to see here, but which are not highly important. Otherwise utACK da21294

dr-orlovsky · 2021-01-18T11:36:59Z

src/util/amount.rs

@@ -933,23 +954,46 @@ pub mod serde {
            //! Serialize and deserialize [Optoin<Amount>] as real numbers denominated in satoshi.


Not related to this PR, but a typo nearby

Suggested change

//! Serialize and deserialize [Optoin<Amount>] as real numbers denominated in satoshi.

//! Serialize and deserialize [Option<Amount>] as real numbers denominated in satoshi.

Or even better

Suggested change

//! Serialize and deserialize [Optoin<Amount>] as real numbers denominated in satoshi.

//! Serialize and deserialize `Option<`[`Amount`]`>` as real numbers denominated in satoshi.

dr-orlovsky · 2021-01-18T11:46:55Z

src/util/amount.rs

+        let t: T = serde_json::from_str("{\"amt\": 250000000, \"samt\": -250000000}").unwrap();
+        assert_eq!(t, with);
+
+        let t: T = serde_json::from_str("{}").unwrap();


Shouldn't we try from_str(r#{"amt": null, "samt": null}#) here as well, to make sure it's covered?

We could try that explicitly, but that is already tested in the Roundtrip test code I added :)

+1 for trying explicitly, but nit

yeah given outstanding acks maybe merge and add a test when these tests get fixed up with the max value fixes.

src/util/amount.rs

dr-orlovsky · 2021-01-18T11:48:24Z

src/util/amount.rs

+        let with = T {
+            amt: Some(Amount::from_sat(2__500_000_00)),
+            samt: Some(SignedAmount::from_sat(-2__500_000_00)),
+        };


Not necessary, but I would like to test +/-21_000_000__000_000_00 as an edge case

from IRC; follow up work to this is likely going to reduce max_value to 21e6, which will require changing some of these things.

Is there a certain reason to reduce the max value to anything below the actual maximum representation? IIRC both Amount types have safe math traits implemented on them.

Just to day that things like "total received" etc could exceed 21M and can still be denominated in sats/btc.

Yes, currently the max value is above the representation as a json number so serialization / deserialization can fail

See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER

This number is still plenty big, 4.2 times 21 million * 100 million sats. If you would like to support more revenue than that -- which perhaps we should! -- we'd need to look into string-ized numbers.

sgeisler

tACK da21294

JeremyRubin · 2021-02-24T00:14:12Z

Summary:

@thomaseizinger utack da21294
@dr-orlovsky utack da21294
@sgeisler tACK da21294

Probably ok to merge?

sgeisler · 2021-02-24T11:32:04Z

I just wanted to merge, but my spidey sense was tingling: we historically we didn't care as much about API breaks because most releases were breaking. But since we want to get away from that we need to pay more attention to API break. While I think this could be merged in a 0.27.0 staging branch it can't in 0.26.1 which we target to release next.

SerdeAmount is a public trait, so adding functions to it is a breaking change.

We need to pay more attention to this on the account that none of the more than three reviewers noticed that.

dr-orlovsky · 2021-02-24T19:05:44Z

Personally I'm ok with having only major releases; what I'd like to have is a more releases per year - not once a quarter or twice a year, especially while we will need to get a lot of changes (Taproot, PSBT2, refactoring). And it seems that minor versions may help with that.

sgeisler · 2021-02-24T21:22:53Z

Some downstream users want fewer API breaks and I think we should try to respect that. The question is how to achieve it without making others depending on new APIs wait for too long. Personally I've been able to work around that by just replacing the dependency with a git version workspace-wide in the past. If that's not a good option for you (e.g. because you want to release to crates.io) you'll have to find a way to make either @apoelstra or @TheBlueMatt cut releases more often.

Since there doesn't seem to be a threshold-signed-release workflow, there's always a set of people with a key each that can make releases unilaterally, so that set has to be kept small for security reasons. Maybe we could build a CI tool that releases the crate if enough authorized people sign a tag (is that even possible to have multiple sigs on a tag?), but I doubt Andrew trusts CI enough for that 😅 (except maybe if it ran on his server).

JeremyRubin · 2021-02-24T22:48:26Z

Personally I think it's OK since the breaking API change:

Behind a feature flag (serde)
is fixing a bug that could cause serious issues for users of the library
A trait that a user has no reason to ever implement on their own types.

Much better to have a break cause compilation to fail than to have users compile code that is incorrect.

That said:

We should probably make more judicious use of trait seals so we can expose traits as pub without allowing users of the library to implement them
We could also not touch SerdeAmount and implement these methods in SerdeAmountExt to prevent the break here... not sure that is worth it though.

JeremyRubin · 2021-02-24T22:51:20Z

BTW I think it is also a breaking change to fix the bug in the first place...

JeremyRubin · 2021-02-26T04:02:47Z

@sgeisler could you say more on "Personally I've been able to work around that by just replacing the dependency with a git version workspace-wide in the past." btw?

I'm currently using patches to do this and it breaks when a release comes out... are you setting the minor version to like 999?

sgeisler · 2021-02-26T11:28:22Z

@JeremyRubin I never got rid of the annoyances patching causes, they were always small enough to grudgingly accept them. I typically had branches with all the things merged that I had lined up as PRs for upstream. When the next release came I could typically remove the patch. It gets more annoying the deeper in the dependency tree the changes are though, I'm scared of needing changes to bitcoin_hashes for that reason 😬

broken Option<SerdeAmount> serializer.

JeremyRubin · 2021-02-28T17:23:11Z

I've added a patch which I think strikes a decent compromise.

This leaves SerdeAmount and ser/de for SerdeAmount untouched. However, it introduces a new trait SerdeAmountForOpt which is a required bound just for the broken opt module.

Therefore it is a breaking API change which should only impact users who are already depending on the broken code, and even then, it is only impacting the subset of users who are both depending on the broken code and bothered to implement the SerdeAmount trait on their own type (which btw, they could likely just add a derive macro on if it is their type rather than using the SerdeAmount trait, which is also clearly documented that:

     /// This trait is used only to avoid code duplication and naming collisions
     /// of the different serde serialization crates.

so I think any user implementing that would surely have some pause...
).

I don't think it makes sense to wait for a breaking API update -- unless we adopt a paradigm to do breaking API updates per-bug -- as it will decrease the likelyhood that a user can easily get working code because it will make them likely take a number of other breaking changes they may or may not want to accept.

edit: to be fair, I see the value in keeping strict with SemVer. I think that it doesn't make sense to me to artificially limit the rate at which we can fix bugs to match a release schedule. I see the value in releasing as many compatible changes as possible before breaking, but I think ultimately continuing to ship known broken code is a greater harm to users.

sgeisler

I think this is a reasonable compromise.

tACK a0c7f53

Test log:

Feb 28 19:51:22.057  INFO testinator: Installing rust toolchain 'nightly'
Feb 28 19:51:52.719  INFO testinator: Installing rust toolchain 'stable'
Feb 28 19:51:53.316  INFO testinator: Installing rust toolchain '1.29.0'
Feb 28 19:51:54.381  INFO testinator: Preparing environment for rust nightly tests (9 configurations)
Feb 28 19:51:54.382  INFO testinator: Preparing environment for rust stable tests (8 configurations)
Feb 28 19:51:54.382  INFO testinator: Preparing environment for rust 1.29.0 tests (8 configurations)
Feb 28 19:54:13.660  INFO testinator: Running rust 1.29.0 tests in /tmp/rust-bitcoin-1.29.0.HPRt5Wzr5LKu/rust-bitcoin
Feb 28 19:54:13.660 DEBUG testinator: Generating lock file with rust=1.29.0
Feb 28 19:54:13.663  INFO testinator: Running rust stable tests in /tmp/rust-bitcoin-stable.VmRlJTLjL6ft/rust-bitcoin
Feb 28 19:54:13.665  INFO testinator: Running rust nightly tests in /tmp/rust-bitcoin-nightly.5Y9zpn6pmmZU/rust-bitcoin
Feb 28 19:54:21.512 DEBUG testinator: Pinning cc to 1.0.41
Feb 28 19:54:21.658 DEBUG testinator: Pinning serde to 1.0.98
Feb 28 19:54:21.790 DEBUG testinator: Pinning serde_derive to 1.0.98
Feb 28 19:54:21.929 DEBUG testinator: Pinning byteorder to 1.3.4
Feb 28 19:55:27.365  INFO testinator: Test rust=nightly, features=[secp-recovery] succeeded!
Feb 28 19:55:29.862  INFO testinator: Test rust=stable, features=[secp-recovery] succeeded!
Feb 28 19:56:09.576  INFO testinator: Test rust=1.29.0, features=[secp-recovery] succeeded!
Feb 28 19:56:10.404  INFO testinator: Test rust=nightly, features=[use-serde] succeeded!
Feb 28 19:56:20.737  INFO testinator: Test rust=stable, features=[use-serde] succeeded!
Feb 28 19:56:33.779  INFO testinator: Test rust=nightly, features=[base64] succeeded!
Feb 28 19:56:50.637  INFO testinator: Test rust=stable, features=[base64] succeeded!
Feb 28 19:57:03.381  INFO testinator: Test rust=nightly, features=[secp-lowmemory] succeeded!
Feb 28 19:57:21.638  INFO testinator: Test rust=stable, features=[secp-lowmemory] succeeded!
Feb 28 19:57:25.482  INFO testinator: Test rust=nightly, features=[rand] succeeded!
Feb 28 19:57:48.830  INFO testinator: Test rust=nightly, features=[unstable] succeeded!
Feb 28 19:57:50.446  INFO testinator: Test rust=stable, features=[rand] succeeded!
Feb 28 19:58:06.532  INFO testinator: Test rust=1.29.0, features=[use-serde] succeeded!
Feb 28 19:58:34.711  INFO testinator: Test rust=nightly, features=[bitcoinconsensus] succeeded!
Feb 28 19:58:46.396  INFO testinator: Test rust=stable, features=[bitcoinconsensus] succeeded!
Feb 28 19:59:21.274  INFO testinator: Test rust=1.29.0, features=[base64] succeeded!
Feb 28 19:59:21.751  INFO testinator: Test rust=nightly, features=[secp-recovery,use-serde,base64,secp-lowmemory,rand,unstable,bitcoinconsensus] succeeded!
Feb 28 19:59:36.468  INFO testinator: Test rust=stable, features=[secp-recovery,use-serde,base64,secp-lowmemory,rand,bitcoinconsensus] succeeded!
Feb 28 19:59:43.941  INFO testinator: Test rust=nightly, features=[] succeeded!
Feb 28 20:00:02.211  INFO testinator: Test rust=stable, features=[] succeeded!
Feb 28 20:00:11.653  INFO testinator: Test rust=1.29.0, features=[secp-lowmemory] succeeded!
Feb 28 20:00:52.745  INFO testinator: Test rust=1.29.0, features=[rand] succeeded!
Feb 28 20:01:43.698  INFO testinator: Test rust=1.29.0, features=[bitcoinconsensus] succeeded!
Feb 28 20:02:31.886  INFO testinator: Test rust=1.29.0, features=[secp-recovery,use-serde,base64,secp-lowmemory,rand,bitcoinconsensus] succeeded!
Feb 28 20:03:19.662  INFO testinator: Test rust=1.29.0, features=[] succeeded!
Feb 28 20:03:22.553  INFO testinator: Fuzzing deserialize_script
Feb 28 20:04:36.171  INFO testinator: Successfully fuzzed deserialize_script
Feb 28 20:04:36.171  INFO testinator: Fuzzing uint128_fuzz
Feb 28 20:05:38.113  INFO testinator: Successfully fuzzed uint128_fuzz
Feb 28 20:05:38.113  INFO testinator: Fuzzing deserialize_amount
Feb 28 20:06:39.142  INFO testinator: Successfully fuzzed deserialize_amount
Feb 28 20:06:39.142  INFO testinator: Fuzzing deserialize_transaction
Feb 28 20:07:41.200  INFO testinator: Successfully fuzzed deserialize_transaction
Feb 28 20:07:41.200  INFO testinator: Fuzzing deser_net_msg
Feb 28 20:08:44.168  INFO testinator: Successfully fuzzed deser_net_msg
Feb 28 20:08:44.168  INFO testinator: Fuzzing deserialize_address
Feb 28 20:09:45.174  INFO testinator: Successfully fuzzed deserialize_address
Feb 28 20:09:45.174  INFO testinator: Fuzzing deserialize_block
Feb 28 20:10:47.233  INFO testinator: Successfully fuzzed deserialize_block
Feb 28 20:10:47.233  INFO testinator: Fuzzing outpoint_string
Feb 28 20:11:49.192  INFO testinator: Successfully fuzzed outpoint_string
Feb 28 20:11:49.192  INFO testinator: Fuzzing deserialize_psbt
Feb 28 20:12:52.139  INFO testinator: Successfully fuzzed deserialize_psbt

sgeisler · 2021-02-28T18:51:08Z

src/util/amount.rs

@@ -875,13 +875,30 @@ pub mod serde {

    /// This trait is used only to avoid code duplication and naming collisions
    /// of the different serde serialization crates.
+    ///
+    /// TODO: Add the private::Sealed bound in next breaking release


I hope someone will remember 😆

apoelstra · 2021-03-12T19:25:20Z

Agree that this obscure enough (and the existing code is broken) that we can put this in a non-breaking release.

Regarding the general "we should have releases more often", I agree. I think we can probably speed up a bit now that we have some more maintainers. But historically, only one of {me, steven, matt} have been available in any given month which results in a ton of single-ACK PRs building up.

I appreciate that downstream users do not like our pace of breaking changes. I don't think we'll be able to make them happy in the next year or two. The bitcoin space is moving too quickly. There are going to be breaking changes for PSBT2, for bech32m, and for Taproot. There is going to be a slew of breaking changes when we flatten the module hierarchy here and clean up our error types. Between Satoshi and Rust 1.29, there will be an ongoing cadence of breaking changes as we adjust how we compromise on impossible API decisions.

stevenroose · 2021-03-13T19:30:58Z

src/util/amount.rs

+        let with = T {
+            amt: Some(Amount::from_sat(2__500_000_00)),
+            samt: Some(SignedAmount::from_sat(-2__500_000_00)),
+        };


Is there a certain reason to reduce the max value to anything below the actual maximum representation? IIRC both Amount types have safe math traits implemented on them.

Just to day that things like "total received" etc could exceed 21M and can still be denominated in sats/btc.

stevenroose · 2021-03-13T19:31:36Z

src/util/amount.rs

+        let t: T = serde_json::from_str("{\"amt\": 250000000, \"samt\": -250000000}").unwrap();
+        assert_eq!(t, with);
+
+        let t: T = serde_json::from_str("{}").unwrap();


+1 for trying explicitly, but nit

stevenroose · 2021-03-13T19:32:53Z

Also +1 for releasing this as a minor release as it's a bug fix IMO.

stevenroose · 2021-03-13T19:34:43Z

And yeah we should probably have a top-level private::Sealed trait that we can add to all internal traits so that we can discard changes that are in principle breaking, but not really.

Fix Optional Amount Serialization

da21294

thomaseizinger previously approved these changes Jan 14, 2021

View reviewed changes

dr-orlovsky reviewed Jan 18, 2021

View reviewed changes

dr-orlovsky previously approved these changes Jan 18, 2021

View reviewed changes

dr-orlovsky added this to the 0.26.1 milestone Jan 30, 2021

sgeisler previously approved these changes Feb 21, 2021

View reviewed changes

sgeisler added the API break This PR requires a version bump for the next release label Feb 24, 2021

sgeisler modified the milestones: 0.26.1, 0.27.0 Feb 24, 2021

Localize breaking changes of fixing the Amount serialization to only the

a0c7f53

broken Option<SerdeAmount> serializer.

JeremyRubin dismissed stale reviews from sgeisler, dr-orlovsky, and thomaseizinger via a0c7f53 February 28, 2021 17:14

sgeisler approved these changes Feb 28, 2021

View reviewed changes

sgeisler requested a review from apoelstra February 28, 2021 19:46

sgeisler modified the milestones: 0.27.0, 0.26.1 Feb 28, 2021

sgeisler added the bug label Feb 28, 2021

stevenroose approved these changes Mar 13, 2021

View reviewed changes

apoelstra merged commit 20f1543 into rust-bitcoin:master Mar 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Optional Amount Serialization #552

Fix Optional Amount Serialization #552

JeremyRubin commented Jan 14, 2021

thomaseizinger left a comment

thomaseizinger Jan 14, 2021

thomaseizinger Jan 14, 2021

JeremyRubin commented Jan 14, 2021

dr-orlovsky left a comment

dr-orlovsky Jan 18, 2021

dr-orlovsky Jan 18, 2021

dr-orlovsky Jan 18, 2021

JeremyRubin Jan 18, 2021

stevenroose Mar 13, 2021

JeremyRubin Mar 13, 2021

dr-orlovsky Jan 18, 2021

JeremyRubin Jan 18, 2021

stevenroose Mar 13, 2021

JeremyRubin Mar 13, 2021

sgeisler left a comment •

edited

JeremyRubin commented Feb 24, 2021

sgeisler commented Feb 24, 2021

dr-orlovsky commented Feb 24, 2021

sgeisler commented Feb 24, 2021

JeremyRubin commented Feb 24, 2021

JeremyRubin commented Feb 24, 2021

JeremyRubin commented Feb 26, 2021

sgeisler commented Feb 26, 2021

JeremyRubin commented Feb 28, 2021 •

edited

sgeisler left a comment

sgeisler Feb 28, 2021

apoelstra commented Mar 12, 2021

stevenroose Mar 13, 2021

stevenroose Mar 13, 2021

stevenroose commented Mar 13, 2021

stevenroose commented Mar 13, 2021

		@@ -933,23 +954,46 @@ pub mod serde {
		//! Serialize and deserialize [Optoin<Amount>] as real numbers denominated in satoshi.

	//! Serialize and deserialize [Optoin<Amount>] as real numbers denominated in satoshi.
	//! Serialize and deserialize [Option<Amount>] as real numbers denominated in satoshi.

	//! Serialize and deserialize [Optoin<Amount>] as real numbers denominated in satoshi.
	//! Serialize and deserialize `Option<`[`Amount`]`>` as real numbers denominated in satoshi.

Fix Optional Amount Serialization #552

Fix Optional Amount Serialization #552

Conversation

JeremyRubin commented Jan 14, 2021

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeremyRubin commented Jan 14, 2021

dr-orlovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgeisler left a comment • edited

Choose a reason for hiding this comment

JeremyRubin commented Feb 24, 2021

sgeisler commented Feb 24, 2021

dr-orlovsky commented Feb 24, 2021

sgeisler commented Feb 24, 2021

JeremyRubin commented Feb 24, 2021

JeremyRubin commented Feb 24, 2021

JeremyRubin commented Feb 26, 2021

sgeisler commented Feb 26, 2021

JeremyRubin commented Feb 28, 2021 • edited

sgeisler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra commented Mar 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenroose commented Mar 13, 2021

stevenroose commented Mar 13, 2021

sgeisler left a comment •

edited

JeremyRubin commented Feb 28, 2021 •

edited