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 a consensus::deserialize_hex function #2039

Merged
merged 1 commit into from Mar 17, 2024
Merged

Add a consensus::deserialize_hex function #2039

merged 1 commit into from Mar 17, 2024

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Aug 29, 2023 •
edited

We have serialize_hex and deserialize but no deserialize_hex, add it.

Move the IterReader out of consensus::serde to the consensus module.

Add some additional logic to the DecodeError, I'm not sure why this wasn't there before?

Use the HexSliceToBytesIter by way of the IterReader to deserialize an arbitrary hex string. Add unit tests to check that we consume all bytes when deserializing a fixed size object (a transaction).

@tcharding
Copy link
Member Author

Will need #2036

tcharding added a commit to tcharding/rust-bitcoincore-rpc that referenced this pull request Aug 29, 2023
@tcharding
WIP: Upgrade the bitcoin dependency
75cf7bc 
Upgrade the `bitcoin` dependency to the soon-to-be-released version.

Doing this patch lead to:

- rust-bitcoin/rust-bitcoin#2038
- rust-bitcoin/rust-bitcoin#2039

Once they merge and we release then this PR can come off WIP.
@tcharding tcharding mentioned this pull request Aug 29, 2023
Closed
@tcharding tcharding force-pushed the 08-29-deserialize-hex branch 2 times, most recently from f68da07 to f71d250 Compare August 31, 2023 03:34
@tcharding
Copy link
Member Author

I'm now not sure this is correct because it does not have the same behaviour in regards to erroring if all data is not consumed. However doing so needs a refactor which implies I have to change the error handling because I've used the "new" style. I'm inclined to put this to draft and do it after the release ...

@apoelstra
Copy link
Member

Drafting sounds good to me. deserialize_hex should definitely error if it doesn't consume all the bytes. If users want to do partial reads of a stream they should be using the consensus_decode trait.

@tcharding
Copy link
Member Author

Cool, thanks.

@tcharding tcharding marked this pull request as draft September 3, 2023 15:48
@stevenroose
Copy link
Collaborator

Yes I have wanted this SO MUCH for SO LONG :) :)

@tcharding
Copy link
Member Author

I took it from you rust-bitcoincore-rpc crate. After thinking "what a lazy bastard, why didn't he upstream this" :)

stevenroose
stevenroose reviewed Oct 22, 2023
View reviewed changes
Copy link
Collaborator

@stevenroose stevenroose left a comment

Choose a reason for hiding this comment

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

Sorry, had these comments here pending in the review thing in github, not sure they're still relevant.

bitcoin/src/consensus/mod.rs Outdated
#[derive(Debug)]
pub enum DecodeHexError {
/// Hex decoding error.
Decode(HexToBytesError),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hex/HexFormat?

bitcoin/src/consensus/mod.rs Outdated
/// Hex decoding error.
Decode(HexToBytesError),
/// Consensus deserialization error.
Deser(encode::Error),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm that short code isn't super helpful. Formatting/Serialization/Encoding or something.

@stevenroose
Copy link
Collaborator

I took it from you rust-bitcoincore-rpc crate. After thinking "what a lazy bastard, why didn't he upstream this" :)

Vaguely remember something like "we don't know how to deal with errors when it comes to hex and don't want to think about it". Which I agreed with at the time, we didn't do much hex other than hash types in the hashes::hex module and converting to hex never gives errors while parsing it can.

But glad it's making it in. Tbh on principle we should have neither of them and users should just use a hex crate to do hex, but well, it's so common in Bitcoin to represent txs as a very specific type of hex (non-prefixed lowercase) that we can get away with it IMO.

@tcharding
Copy link
Member Author

Cool, thanks for looking. I'll come back to this once v0.31.0 is out.

@Kixunil
Copy link
Collaborator

Kixunil commented Oct 26, 2023

it's so common in Bitcoin to represent txs as a very specific type of hex (non-prefixed lowercase) that we can get away with it

Yeah, I would even consider impl FromStr for Transaction

@tcharding tcharding marked this pull request as ready for review December 6, 2023 22:48
@tcharding tcharding marked this pull request as draft December 6, 2023 22:53
@tcharding
Copy link
Member Author

tcharding commented Dec 6, 2023 •
edited

Needs hex to use new io crate.

EDIT: Turns out I was wrong, PR now uses a custom iterator (originally from consensus::serde.

@Kixunil
Copy link
Collaborator

Kixunil commented Dec 7, 2023

Ah, interesting, so IO is actually useful in hex.

@stevenroose
Copy link
Collaborator

Damn, I was using this method just now and then realized it doesn't exist yet..

@tcharding tcharding mentioned this pull request Jan 8, 2024
Closed
Kixunil
Kixunil reviewed Jan 15, 2024
View reviewed changes
Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

core2 was removed so this can proceed.

bitcoin/src/consensus/encode.rs Outdated Show resolved Hide resolved
@github-actions github-actions bot added C-bitcoin PRs modifying the bitcoin crate C-hashes PRs modifying the hashes crate labels Feb 23, 2024
@tcharding
Copy link
Member Author

This is WIP but showcases that we only need Read in hex not BufRead.

@tcharding tcharding mentioned this pull request Feb 23, 2024
Closed
@tcharding tcharding force-pushed the 08-29-deserialize-hex branch 2 times, most recently from 805aa5c to a6323f4 Compare February 27, 2024 23:20
@coveralls
Copy link

coveralls commented Feb 27, 2024 •
edited

Pull Request Test Coverage Report for Build 8272603794

Details

  • 37 of 101 (36.63%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.1%) to 84.038%
Changes Missing Coverage Covered Lines Changed/Added Lines %
bitcoin/src/consensus/encode.rs 18 35 51.43%
bitcoin/src/consensus/mod.rs 19 66 28.79%
Files with Coverage Reduction New Missed Lines %
bitcoin/src/consensus/serde.rs 1 43.87%
Totals Coverage Status
Change from base Build 8269012797: -0.1%
Covered Lines: 19685
Relevant Lines: 23424
💛 - Coveralls

@tcharding
Copy link
Member Author

Needs #2512

@tcharding tcharding marked this pull request as ready for review March 12, 2024 01:58
@tcharding
Copy link
Member Author

Might need #2573

@tcharding tcharding mentioned this pull request Mar 12, 2024
Open
@tcharding tcharding added this to the 0.32.0 milestone Mar 12, 2024
@tcharding tcharding mentioned this pull request Mar 12, 2024
Draft
@tcharding
Implement consensus deserialize_hex
16a8137 
We have `serialize_hex` and `deserialize` but no `deserialize_hex`, add it.

Move the `IterReader` out of `consensus::serde` to the `consensus`
module.

Add some additional logic to the `DecodeError`, I'm not sure why this
wasn't there before?

Use the `HexSliceToBytesIter` by way of the `IterReader` to deserialize
an arbitrary hex string. Add unit tests to check that we consume all
bytes when deserializing a fixed size object (a transaction).
@tcharding
Copy link
Member Author

Rebased for good measure to pick up #2573, there are no acks anyways. No other changes.

@tcharding tcharding removed C-hashes PRs modifying the hashes crate C-base58 labels Mar 14, 2024
@tcharding
Copy link
Member Author

Labels were stale.

apoelstra
apoelstra approved these changes Mar 16, 2024
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.

ACK 16a8137

sanket1729
sanket1729 approved these changes Mar 17, 2024
View reviewed changes
Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

ACK 16a8137

@apoelstra apoelstra merged commit 7b7461c into rust-bitcoin:master Mar 17, 2024
187 checks passed
@tcharding tcharding deleted the 08-29-deserialize-hex branch March 19, 2024 03:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevenroose stevenroose stevenroose left review comments

@Kixunil Kixunil Kixunil left review comments

@RCasatta RCasatta RCasatta left review comments

@apoelstra apoelstra apoelstra approved these changes

@sanket1729 sanket1729 sanket1729 approved these changes

Assignees
No one assigned
Labels
C-bitcoin PRs modifying the bitcoin crate
Projects
None yet
Milestone
0.32.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@tcharding @apoelstra @stevenroose @Kixunil @coveralls @RCasatta @sanket1729