Make uint types (un)serializable #511

shesek · 2020-11-03T10:01:57Z

No description provided.

And keep the latest validation status on the store cache. NOTE: This commit depends on a rust-bitcoin fork with serializable uint types. A PR was sent upstream: rust-bitcoin/rust-bitcoin#511

apoelstra

utack

elichai · 2020-11-03T22:47:06Z

We don't use serde_derive so sadly you'll have to impl it yourself (I recommend either via one of the macros here: https://github.com/rust-bitcoin/rust-bitcoin/blob/7c47c9a341f5665f89d0c1ab6e958e365bff949b/src/internal_macros.rs or if nothing fits (I don't remember what's in there anymore) using macro-expand and then prettifying it)

shesek · 2020-11-04T00:14:55Z

@elichai ah-ah, I see. It was working when I tried because serde_derive is in the dev-dependencies. Will look into changing that to use one of the existing macros.

shesek · 2020-11-04T06:04:03Z

@elichai None of the existing macros seem to fit, I ended up implementing a custom serializer/deserializer by hand at 0e9672e.

And keep the latest validation status on the store cache. NOTE: This commit depends on a rust-bitcoin fork with serializable uint types. A PR was sent upstream: rust-bitcoin/rust-bitcoin#511

apoelstra · 2020-11-09T22:20:18Z

Can you add a unit test which parses some fixed json vectors? And covers some error/edge cases?

shesek · 2020-11-10T12:18:39Z

@apoelstra Added some fixed vectors and error cases. Did you have any particular edge cases to test in mind?

And keep the latest validation status on the store cache. NOTE: This commit depends on a rust-bitcoin fork with serializable uint types. A PR was sent upstream: rust-bitcoin/rust-bitcoin#511

shesek · 2020-12-17T10:04:40Z

Is there anything else needed to get this through? :)

And keep the latest validation status on the store cache. NOTE: This commit depends on a rust-bitcoin fork with serializable uint types. A PR was sent upstream: rust-bitcoin/rust-bitcoin#511

sgeisler

What's the motivation for using our internal representation (64bit words) for the serde implementation? This could lead to some weird mixed-endianess in binary serde implementations and doesn't look pretty as json. I think treating these bignum types as byte arrays with a hex representations in human redable serde backends would be best if there are no other compatibility requirements.

Also: travis doesn't seem happy, something failed for rust 1.29.0.

shesek · 2020-12-30T04:48:02Z

@sgeisler No particular reason, I like hex representation better too. I changed to it and fixed compatibility with Rust 1.29. (forced-pushed over the previous implementation, which is available for reference at 202011-uint-serde-jsonarray.)

I wasn't sure whether the new from_be_slice() should enforce the length and how. I kept the commit that changed it to return a Result separate, let me know what you think and I'll squash it in if it seems acceptable.

An alternative to adding from_be_slice() is to change the existing from_be_bytes() to take an AsRef<u8> which will work for both slices and arrays, but this will lose the type checking for the array length.

shesek · 2020-12-30T04:49:51Z

Travis appears to be stuck due to some unrelated issue, it works on my fork.

sgeisler · 2021-01-04T15:02:10Z

src/util/uint.rs

+                S: $crate::serde::Serializer,
+            {
+                use $crate::hashes::hex::ToHex;
+                serializer.serialize_str(&self.to_be_bytes().to_hex())


This appears to always serialize as a string, even if the output format is binary. This results in suboptimal encodings. We avoid this in other areas of the code by switching the encoding strategy based on the human readability.

Agreed, implemented in d0eed01.

Testing this required introducing a new (dev only) dependency on a serde binary serializer (bincode). I wasn't sure if that's desirable or not so I kept it in a separate commit for now, will squash if it is.

And keep the latest validation status on the store cache. NOTE: This commit depends on a rust-bitcoin fork with serializable uint types. A PR was sent upstream: rust-bitcoin/rust-bitcoin#511

sgeisler

Looks good now. I don't think a new dev-dependency is much of a problem, especially since it seems to build fine with 1.29.0.

ACK 1be22c3

dr-orlovsky

Sorry for being late, but I see space for further improvements over a type system here. Had a lot of issues with my own code when rust-bitcoin does not implement errors properly.

dr-orlovsky · 2021-01-14T17:47:48Z

src/util/uint.rs

    );
 }

 construct_uint!(Uint256, 4);
 construct_uint!(Uint128, 2);

+/// Uint error
+#[derive(Debug)]


I think we can be opportunistic here, as Rust API guidelines suggest:

Suggested change

#[derive(Debug)]

#[derive(Debug, PartialEq, PartialOrd, Clone, Copy, Hash)]

Good point, I always forget theses derives myself 🙈 do you know if there is some way to warn about missing (standard) derives?

Even if there is a way, I never heard of it :(

Oh no, maybe clippy may help?

src/util/uint.rs

shesek · 2021-01-14T20:23:55Z

I switched to a single-variant error type and implemented the suggested traits, apart from Error ~~which I noticed isn't implemented anywhere else in rust-bitcoin. Should I implement that too?~~ (#511 (comment))

src/util/uint.rs

dr-orlovsky · 2021-01-14T20:30:35Z

Not sure about Error whether it would compile on 1.29. I assume yes, and the reason it is absent everywhere is that it was not present before we upgraded to 1.29

thomaseizinger · 2021-01-14T22:37:12Z

apart from Error which I noticed isn't implemented anywhere else in rust-bitcoin

That doesn't seem to be true? GitHub search returns 12 results for "impl error::Error": https://github.com/rust-bitcoin/rust-bitcoin/search?q=%22impl+error%3A%3AError%22 (and that might be missing some where it is imported differently)

Please implement std::error::Error otherwise working with libraries like anyhow downstream is quite painful.

shesek · 2021-01-15T14:59:38Z

@thomaseizinger Oops, you're of course correct, not sure how my search missed that. I will implement this and the other suggested changes.

shesek · 2021-01-16T11:07:47Z

I implemented Error and Eq.

dr-orlovsky

I still believe we need Ord when we have Eq and PartialOrd. The rest looks fine for me now

sgeisler · 2021-01-17T22:28:40Z

@dr-orlovsky I don't think that's the case mathematically speaking (it probably still makes sense practically in this case if it's possible).

Let's imagine the set of all binary strings. Eq is implemented as expected: compare bit values (and implicitly legth). PartialOrd is implemented that it returns Some(Equal) if a == b, Some(Greater) if a is a prefix of b, Some(Less) if b is a prefix of a and None otherwise. This is a valid impl as it's both asymmetric and transitive, compatible with the Eq impl, but not Ord.

That's obviously a contrived example, but I don't see a strict requirement for Ord just because we have Eq and PartialOrd.

sanket1729 · 2021-01-18T00:09:00Z

On one side, if we can derive something for free, we should derive it.

On the other side, Display, Clone, Debug, Error, and Eq(implying PartialEq) are the only traits that I can foresee being used.

I don't think it's important what we end up doing, but we should decide one thing and be consistent with that for all the Error types in the repo. This would help when we wrap Error of one type as an enum variant of another Enum and try to auto derive on that.

dr-orlovsky · 2021-01-18T11:22:16Z

The use for Ord are the situations where you need to collect errors from multiple attempts of processing into a HashMap, mapping them to the item. Thus, if we can derive Ord I think we should do it, and not only here, but on all Error types - which is a matter of other PRs of course (I did one already #551)

@sgeisler thanks for explanation, that is a valid case.

sanket1729 · 2021-01-21T20:13:42Z

I think HashMap should only Hash and Eq, but BTreeMap will require Ord. It may be possible that we have extra work for manually impl Ord because the error contains f64 for example.

That being said, we can try everything and see how it goes. We can continue this in #555

dr-orlovsky · 2021-01-21T22:01:40Z

I do agree with @apoelstra in #555 (comment) that we should implement both Ord (and PartialOrd of course) where is possible - and this is also Rust API Guidelines recommendation.

Had plenty of pain not being able to use datatypes in BTree* when I need to put them there.

shesek · 2021-01-21T22:28:50Z

I added Ord. I see that Hash and Default were also mentioned in #555, are they relevant for error types?

dr-orlovsky

Thank you and sorry for being insistent. utACK 55657cb

dr-orlovsky · 2021-01-21T23:36:08Z

I think here Default just makes not sense: you cen't have a "default" Error. And the type already has Hash derived on it

sanket1729

tACk a1e98a6

sgeisler

utACK a1e98a6

@sanket1729 are you still against merging without a more thorough test than CI (=currently @apoelstra's test suite)? I'm currently building my own setup (cargo-all-features and some bash, not as fancy but I'll understand it xD). So I hope we can get stuff merged again soon.

sanket1729 · 2021-02-05T16:45:15Z

Yeah, let's merge this.

And keep the latest validation status on the store cache. NOTE: This commit depends on a rust-bitcoin fork with serializable uint types. A PR was sent upstream: rust-bitcoin/rust-bitcoin#511

shesek mentioned this pull request Nov 3, 2020

SPV multi-server cross-validation Blockstream/gdk#59

Merged

apoelstra previously approved these changes Nov 3, 2020

View reviewed changes

shesek dismissed apoelstra’s stale review via 0e9672e November 4, 2020 05:59

shesek force-pushed the 202011-uint-serde branch from b5f0121 to 0e9672e Compare November 4, 2020 05:59

shesek force-pushed the 202011-uint-serde branch from 0e9672e to 4a18151 Compare November 7, 2020 01:07

shesek force-pushed the 202011-uint-serde branch from 4a18151 to 7c519fd Compare November 10, 2020 11:49

shesek force-pushed the 202011-uint-serde branch from 64a5961 to c122645 Compare November 10, 2020 12:20

sgeisler reviewed Dec 17, 2020

View reviewed changes

shesek force-pushed the 202011-uint-serde branch 2 times, most recently from 6d089b2 to 1e9705a Compare December 30, 2020 04:36

sgeisler closed this Jan 4, 2021

sgeisler reopened this Jan 4, 2021

sgeisler reviewed Jan 4, 2021

View reviewed changes

sgeisler previously approved these changes Jan 10, 2021

View reviewed changes

shesek dismissed sgeisler’s stale review via 3450ca8 January 11, 2021 05:12

dr-orlovsky suggested changes Jan 14, 2021

View reviewed changes

Switch to a single-variant error type, implement standard derives

0df86b4

shesek dismissed sgeisler’s stale review via 0df86b4 January 14, 2021 20:22

shesek force-pushed the 202011-uint-serde branch from 46acd77 to 0df86b4 Compare January 14, 2021 20:22

dr-orlovsky suggested changes Jan 14, 2021

View reviewed changes

src/util/uint.rs Outdated Show resolved Hide resolved

src/util/uint.rs Show resolved Hide resolved

Implement Error and Eq for ParseLengthError

55657cb

dr-orlovsky suggested changes Jan 16, 2021

View reviewed changes

sgeisler mentioned this pull request Jan 18, 2021

Define and enforce standard derives #555

Closed

Implement Ord for ParseLengthError

a1e98a6

dr-orlovsky approved these changes Jan 21, 2021

View reviewed changes

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

sanket1729 approved these changes Jan 30, 2021

View reviewed changes

sgeisler approved these changes Feb 5, 2021

View reviewed changes

sgeisler merged commit 90f3529 into rust-bitcoin:master Feb 5, 2021

sanket1729 mentioned this pull request Feb 18, 2021

SigHashType: add a method to error on non-standard hashtypes #573

Merged

	#[derive(Debug)]
	#[derive(Debug, PartialEq, PartialOrd, Clone, Copy, Hash)]

Make uint types (un)serializable #511

Make uint types (un)serializable #511

Conversation

shesek commented Nov 3, 2020

apoelstra left a comment

Choose a reason for hiding this comment

elichai commented Nov 3, 2020

shesek commented Nov 4, 2020

shesek commented Nov 4, 2020

apoelstra commented Nov 9, 2020

shesek commented Nov 10, 2020

shesek commented Dec 17, 2020

sgeisler left a comment

Choose a reason for hiding this comment

shesek commented Dec 30, 2020 • edited

shesek commented Dec 30, 2020

sgeisler Jan 4, 2021

Choose a reason for hiding this comment

shesek Jan 5, 2021 • edited

Choose a reason for hiding this comment

sgeisler left a comment

Choose a reason for hiding this comment

dr-orlovsky left a comment

Choose a reason for hiding this comment

dr-orlovsky Jan 14, 2021

Choose a reason for hiding this comment

sgeisler Jan 14, 2021

Choose a reason for hiding this comment

dr-orlovsky Jan 14, 2021 • edited

Choose a reason for hiding this comment

dr-orlovsky Jan 14, 2021

Choose a reason for hiding this comment

shesek commented Jan 14, 2021 • edited

dr-orlovsky commented Jan 14, 2021 • edited

thomaseizinger commented Jan 14, 2021 • edited

shesek commented Jan 15, 2021

shesek commented Jan 16, 2021

dr-orlovsky left a comment

Choose a reason for hiding this comment

sgeisler commented Jan 17, 2021

sanket1729 commented Jan 18, 2021

dr-orlovsky commented Jan 18, 2021 • edited

sanket1729 commented Jan 21, 2021 • edited

dr-orlovsky commented Jan 21, 2021

shesek commented Jan 21, 2021

dr-orlovsky left a comment

Choose a reason for hiding this comment

dr-orlovsky commented Jan 21, 2021

sanket1729 left a comment

Choose a reason for hiding this comment

sgeisler left a comment

Choose a reason for hiding this comment

sanket1729 commented Feb 5, 2021

shesek commented Dec 30, 2020 •

edited

shesek Jan 5, 2021 •

edited

dr-orlovsky Jan 14, 2021 •

edited

shesek commented Jan 14, 2021 •

edited

dr-orlovsky commented Jan 14, 2021 •

edited

thomaseizinger commented Jan 14, 2021 •

edited

dr-orlovsky commented Jan 18, 2021 •

edited

sanket1729 commented Jan 21, 2021 •

edited