Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add special human-readable serde for OutPoint and most bip32 types #271

Merged
merged 5 commits into from Aug 2, 2019
Merged

Add special human-readable serde for OutPoint and most bip32 types #271

merged 5 commits into from Aug 2, 2019

Conversation

stevenroose
Copy link
Collaborator

@stevenroose stevenroose commented May 29, 2019 •
edited

Serialized OutPoint as "<txid>:<vout>" instead of a struct when a human-readable (de)serializer is used, f.e. JSON.

It should be noted that this is a breaking change for people relying on the current JSON serialization of OutPoint as a struct.

This PR is on top of #269, but doesn't have to be.

Closes #202.

@apoelstra
Copy link
Member

This is great. Can you make an analogous PR to rust-elements?

@apoelstra
Copy link
Member

I think we should first get rid of the serde macros. They aren't needed since rustc 0.15.

apoelstra
apoelstra reviewed Jun 6, 2019
View reviewed changes
src/internal_macros.rs Outdated Show resolved Hide resolved
apoelstra
apoelstra reviewed Jun 6, 2019
View reviewed changes
src/internal_macros.rs Outdated Show resolved Hide resolved
@apoelstra
Copy link
Member

Eh, actually there's no reason to get rid of the serde macros yet. ACK mod nit.

@stevenroose stevenroose added API break This PR requires a version bump for the next release enhancement labels Jun 6, 2019
@stevenroose
Copy link
Collaborator Author

Eh, actually there's no reason to get rid of the serde macros yet. ACK mod nit.

Were you talking about using derive for the struct macro?

@stevenroose
Copy link
Collaborator Author

I addressed the collect_str comments. This PR merges #269 as well though. If you want to merge this one before that, I guess I could re-push it standalone.

@stevenroose
Copy link
Collaborator Author

Btw, any idea if it's possible to condense that macro in the two other macros? It's kind of a total code duplicate of serde_struct_impl and serde_string_impl with an if-statement to choose which.

@apoelstra
Copy link
Member

Yeah, serde_struct_impl could be replaced with #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]. But that's somewhat independent of these changes.

No, I don't have any good ideas about how to reduce code duplication :(

@apoelstra apoelstra mentioned this pull request Jun 7, 2019
Closed
@stevenroose stevenroose changed the title Add special human-readable serde for OutPoint Add special human-readable serde for OutPoint and bip32::Extended*Key Jun 8, 2019
apoelstra
apoelstra previously approved these changes Jun 12, 2019
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@stevenroose stevenroose mentioned this pull request Jun 13, 2019
Merged
@dongcarl
Copy link
Member

I'm not too familiar with serde, but I can attest that:

  • The changes in 8d7a1ff isn't more wrong than before
  • The changes in ebe6ace make sense conceptually
  • The changes in 42d3a30 is correct and doesn't change behavior (or binary output if Rust's "free-abstraction" works)

If anyone can attest to the serde code in ebe6ace that'd be great.

@stevenroose stevenroose force-pushed the serde-outpoint branch 3 times, most recently from 1ecc696 to b59aedf Compare July 15, 2019 18:04
@stevenroose stevenroose changed the title Add special human-readable serde for OutPoint and bip32::Extended*Key Add special human-readable serde for OutPoint and most bip32 types Jul 15, 2019
@stevenroose
Copy link
Collaborator Author

If anyone can attest to the serde code in ebe6ace that'd be great.

You should be able to see that it's nothing more than the other two serde impl macros combined with an if serializer.human_readable() condition.

@stevenroose
Copy link
Collaborator Author

Fixed Rust v1.22 compilation.

apoelstra
apoelstra previously approved these changes Jul 22, 2019
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-utACK

dongcarl
dongcarl reviewed Jul 26, 2019
View reviewed changes
src/internal_macros.rs Outdated Show resolved Hide resolved
@apoelstra apoelstra added this to the 0.19 milestone Jul 27, 2019
@stevenroose
Copy link
Collaborator Author

Added small commit to address @dongcarl 's nit.

apoelstra
apoelstra approved these changes Aug 1, 2019
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes look fine

dongcarl
dongcarl approved these changes Aug 2, 2019
View reviewed changes
Copy link
Member

@dongcarl dongcarl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@apoelstra apoelstra merged commit 2e915bf into rust-bitcoin:master Aug 2, 2019
shesek added a commit to Blockstream/electrs that referenced this pull request Oct 1, 2021
@shesek
Serialize the asset's issuance_prevout as an object
0db507d 
As was the case prior to rust-bitcoin/rust-bitcoin#271
Davidson-Souza pushed a commit to Davidson-Souza/rust-bitcoin that referenced this pull request Jul 12, 2023
@TheBlueMatt
Reduce cryptography usage in --cfg=fuzzing
940a51c 
This reduces the usage of real cryptography in --cfg=fuzzing,
specifically replacing the secret->public key derivation with a
simple copy and ECDH with XOR of the public and private parts
(plus a stream of 1s to make a test pass that expected non-0
output).

It leaves secret tweak addition/multiplication as-is.

It also changes the context creation to over-allocate and store
the context flags at the end of the context buffer, allowing us
to easily test context flags in each function.

While it would be nice to have something fancier (eg XOR-based),
its not immediately obvious how to accomplish this, and better to
fix the issues I have than spend too much time on it.

Fixes rust-bitcoin#271.

This partially reverts b811ec1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apoelstra apoelstra apoelstra approved these changes

@dongcarl dongcarl dongcarl approved these changes

Assignees
No one assigned
Labels
API break This PR requires a version bump for the next release enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bip32 serialization/deserialization of ExtendedPrivKey
3 participants
@stevenroose @apoelstra @dongcarl