Modify from_script functions in address.rs to return result #1038

Eunoia1729 · 2022-06-05T14:56:03Z

Modify from_script functions to return result instead of option so that, in case of errors, there is more
information on what went wrong.

Resolves: #1022

tcharding

Good work, I left a few suggestions.

On commit descriptions and titles: the canonical blog post that folks always link to is: https://cbea.ms/git-commit/, IMO you should definitely have read this post once in your life. The better you do all the auxillary things around making code changes the better response you will get from reviewers and the more chance your changes will have of getting merged.

Of note, this PR is not a refactoring - it makes logic changes.

tcharding · 2022-06-05T23:43:10Z

src/util/address.rs

@@ -81,7 +81,9 @@ pub enum Error {
    /// An uncompressed pubkey was used where it is not allowed.
    UncompressedPubkey,
    /// Address size more than 520 bytes is not allowed.
-    ExcessiveScriptSize
+    ExcessiveScriptSize,
+    /// Script which is not a p2pkh, p2sh or witness program.


Suggested change

/// Script which is not a p2pkh, p2sh or witness program.

/// Script is not a p2pkh, p2sh, or witness program.

Thanks @tcharding for your review and feedback!
The link was super helpful and applied the takeaways from it to this PR. Kindly let me know how it is right now and where it could be further improved.

tcharding · 2022-06-05T23:43:25Z

src/util/address.rs

@@ -98,6 +100,7 @@ impl fmt::Display for Error {
            Error::InvalidSegwitV0ProgramLength(l) => write!(f, "a v0 witness program must be either of length 20 or 32 bytes: length={}", l),
            Error::UncompressedPubkey => write!(f, "an uncompressed pubkey was used where it is not allowed"),
            Error::ExcessiveScriptSize => write!(f, "Script size exceed 520 bytes"),
+            Error::UnrecognizedScript => write!(f, "Script is not a p2pk, p2sh or witness program")


Same as above. For the record, error messages should start with a lower case letter because they are often combined with other error messages e.g., "erorr one: error two: error three".

Adding an extra commit to update the error message for Error::ExcessiveScriptSize.

tcharding · 2022-06-05T23:47:16Z

src/util/address.rs

@@ -119,7 +122,8 @@ impl std::error::Error for Error {
            | InvalidWitnessProgramLength(_)
            | InvalidSegwitV0ProgramLength(_)
            | UncompressedPubkey
-            | ExcessiveScriptSize => None,
+            | ExcessiveScriptSize 


This line has trailing whitespace. You should be able to configure what ever system you use to view diffs before pushing them (editor or shell) to highlight trailing white space. In case its useful, I personally view diffs using

git-log-patch () { local commit if (( $# < 1 )) then commit="HEAD~" else commit="$1" fi git log --color=always --patch --reverse "${commit}".. | vimpager }

Along with a shell alias :)

@tcharding Hey, this is good. Thanks a lot! Though I modified it to (~/.gitconfig):

[alias] check = "!f() { git log --color=always --patch --reverse "${1:-HEAD~}"..; }; f"

That pager seems unnecessary BTW, git log pages by default(?) when run on interactive shell.

Oh nice, I started using vimpager because I was loosing colours somewhere but seems you are correct. I can stop using it, cheers.

Thanks for this snippet @dpc and @tcharding ! TIL.

tcharding · 2022-06-05T23:50:04Z

src/util/address.rs

@@ -397,11 +401,11 @@ impl Payload {
            Payload::ScriptHash(ScriptHash::from_inner(hash_inner))
        } else if script.is_witness_program() {
            Payload::WitnessProgram {
-                version: WitnessVersion::from_opcode(opcodes::All::from(script[0])).ok()?,
+                version: WitnessVersion::from_opcode(opcodes::All::from(script[0])).ok().unwrap(),


Typically we never want to use unwrap in non-test code because it panics. If the code path can never be hit use expect and explain why it cannot panic in the argument string.

(I'm intentionally not putting a suggestion for a better way to do this so you can problem solve it yourself :)

Ahh, makes sense!
Kindly let me know if the current change looks good.

nlanson · 2022-06-06T07:43:20Z

src/util/address.rs

@@ -98,6 +100,7 @@ impl fmt::Display for Error {
            Error::InvalidSegwitV0ProgramLength(l) => write!(f, "a v0 witness program must be either of length 20 or 32 bytes: length={}", l),
            Error::UncompressedPubkey => write!(f, "an uncompressed pubkey was used where it is not allowed"),
            Error::ExcessiveScriptSize => write!(f, "Script size exceed 520 bytes"),
+            Error::UnrecognizedScript => write!(f, "Script is not a p2pk, p2sh or witness program")


I believe this should be p2pkh not p2pk.

Suggested change

Error::UnrecognizedScript => write!(f, "Script is not a p2pk, p2sh or witness program")

Error::UnrecognizedScript => write!(f, "Script is not a p2pkh, p2sh or witness program")

Ahh you're right! Thanks for pointing it out, I need more coffee 😄

apoelstra · 2022-06-06T18:50:35Z

ACK 0939ef4 except for all the nits that others have pointed out :)

nlanson · 2022-06-07T06:45:30Z

src/util/address.rs

@@ -397,11 +401,11 @@ impl Payload {
            Payload::ScriptHash(ScriptHash::from_inner(hash_inner))
        } else if script.is_witness_program() {
            Payload::WitnessProgram {


From #1022 (comment), would it benefit to return an error indicating incorrect segwit v0 program length here?

Then we can remove the explicit check in Address::from_script() since it would be done here.

Good point. That way, we can reduce the number of calls to script.is_witness_program() for correct scripts.

tcharding

ACK 2a4f439

Modify from_script functions to return result instead of option so that, in case of errors, there is more information on what went wrong. Resolves: rust-bitcoin#1022

Eunoia1729 · 2022-06-08T19:58:57Z

Changes done in the force push:

Re-based the commit due to merge conflicts.
Added an extra commit to update the error message for Error::ExcessiveScriptSize (unrelated to the issue).

sanket1729

ACK 66e852c. LGTM

tcharding

ACK 66e852c

tcharding · 2022-06-08T23:05:53Z

Hey @Eunoia1729, just to let you know; in rust-bitcoin we only merge PRs when they have two acks and the acks include the commit hash of the tip of the PR. This means that when you force push reviewers have to re-review you work. Combined this with rebasing and it can require a little more concentration from the reviewer. Do force push when you are implementing review suggestions to the PR unless the suggestion can be done as a stand alone patch, this can be a bit hard to judge at times. Just something to keep in mind. You can ask if you want more direction on this sort of stuff. The easier you make life for reviewers the more chance your changes have of being merged.

That was long winded way of saying that you should only typically rebase a PR if there are merge conflicts :)

apoelstra

ACK 2a4f439

apoelstra

ACK 66e852c

…rogramLength and add test 24fdb53 Fix incorrect argument passed to Error::InvalidSegwitV0ProgramLength (eunoia_1729) Pull request description: See also: #995, #1038 ACKs for top commit: sanket1729: utACK 24fdb53. Nice catch apoelstra: ACK 24fdb53 tcharding: ACK 24fdb53 Tree-SHA512: ced78b69054ec81431399a853291c7bad5b1a49d6683b1ac153a0f1449935bb5d75a31e3d86160602064530959a2ddc3c59a2a2ca268252c42a6805253ead9d0

… address.rs to return result 66e852c Update format of ExcessiveScriptSize error message (eunoia_1729) 89bd4b6 Modify from_script functions in address.rs to return result (eunoia_1729) Pull request description: Modify from_script functions to return result instead of option so that, in case of errors, there is more information on what went wrong. Resolves: #1022 ACKs for top commit: sanket1729: ACK 66e852c. LGTM tcharding: ACK 66e852c apoelstra: ACK 66e852c Tree-SHA512: 0d9529aee0a5459351bed2cc8b2c5571736d3293e2931c43d98f53330e9ac5f3d998a19da2b4575af0a3c1c4dcfd5a24c8813390bf6f5492a689c36ebb9cb426

see rust-bitcoin/rust-bitcoin#1038

Eunoia1729 mentioned this pull request Jun 5, 2022

Fix incorrect argument passed to Error::InvalidSegwitV0ProgramLength and add test #1039

Merged

Eunoia1729 force-pushed the fix-1022 branch from 27169da to 0939ef4 Compare June 5, 2022 15:12

tcharding reviewed Jun 5, 2022

View reviewed changes

nlanson reviewed Jun 6, 2022

View reviewed changes

nlanson reviewed Jun 7, 2022

View reviewed changes

Eunoia1729 force-pushed the fix-1022 branch from 0939ef4 to 3d04676 Compare June 7, 2022 07:26

Eunoia1729 changed the title ~~Refactor: make from_script functions in address.rs to return Result~~ Modify from_script functions in address.rs to return result Jun 7, 2022

Eunoia1729 force-pushed the fix-1022 branch from 3d04676 to 2a4f439 Compare June 7, 2022 08:57

tcharding previously approved these changes Jun 7, 2022

View reviewed changes

nlanson previously approved these changes Jun 8, 2022

View reviewed changes

Eunoia1729 added 2 commits June 9, 2022 01:13

Modify from_script functions in address.rs to return result

89bd4b6

Modify from_script functions to return result instead of option so that, in case of errors, there is more information on what went wrong. Resolves: rust-bitcoin#1022

Update format of ExcessiveScriptSize error message

66e852c

Eunoia1729 dismissed stale reviews from nlanson and tcharding via 66e852c June 8, 2022 19:52

Eunoia1729 force-pushed the fix-1022 branch from 2a4f439 to 66e852c Compare June 8, 2022 19:52

Eunoia1729 requested a review from tcharding June 8, 2022 22:29

sanket1729 approved these changes Jun 8, 2022

View reviewed changes

tcharding approved these changes Jun 8, 2022

View reviewed changes

apoelstra approved these changes Jun 8, 2022

View reviewed changes

apoelstra merged commit 3ed3d57 into rust-bitcoin:master Jun 8, 2022

Eunoia1729 deleted the fix-1022 branch June 9, 2022 03:12

Kixunil added the API break This PR requires a version bump for the next release label Aug 1, 2022

Kixunil added this to the 0.29.0 milestone Aug 1, 2022

sander2 added a commit to sander2/polkabtc-clients that referenced this pull request Oct 6, 2022

chore: migration: from_script now returns Result instead of Option

d84f83b

see rust-bitcoin/rust-bitcoin#1038

sander2 added a commit to sander2/polkabtc-clients that referenced this pull request Oct 6, 2022

chore: migration: from_script now returns Result instead of Option

abb5969

see rust-bitcoin/rust-bitcoin#1038

sander2 added a commit to sander2/polkabtc-clients that referenced this pull request Oct 7, 2022

chore: migration: from_script now returns Result instead of Option

eae214d

see rust-bitcoin/rust-bitcoin#1038

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify from_script functions in address.rs to return result #1038

Modify from_script functions in address.rs to return result #1038

Eunoia1729 commented Jun 5, 2022 •

edited

tcharding left a comment

tcharding Jun 5, 2022

Eunoia1729 Jun 7, 2022

tcharding Jun 5, 2022

Eunoia1729 Jun 7, 2022 •

edited

tcharding Jun 5, 2022

tcharding Jun 5, 2022

dpc Jun 6, 2022

tcharding Jun 6, 2022

Eunoia1729 Jun 7, 2022

tcharding Jun 5, 2022

Eunoia1729 Jun 7, 2022 •

edited

nlanson Jun 6, 2022

Eunoia1729 Jun 6, 2022

apoelstra commented Jun 6, 2022

nlanson Jun 7, 2022

Eunoia1729 Jun 7, 2022

tcharding left a comment

Eunoia1729 commented Jun 8, 2022 •

edited

sanket1729 left a comment

tcharding left a comment

tcharding commented Jun 8, 2022

apoelstra left a comment •

edited

apoelstra left a comment

	/// Script which is not a p2pkh, p2sh or witness program.
	/// Script is not a p2pkh, p2sh, or witness program.

	Error::UnrecognizedScript => write!(f, "Script is not a p2pk, p2sh or witness program")
	Error::UnrecognizedScript => write!(f, "Script is not a p2pkh, p2sh or witness program")

Modify from_script functions in address.rs to return result #1038

Modify from_script functions in address.rs to return result #1038

Conversation

Eunoia1729 commented Jun 5, 2022 • edited

tcharding 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

Eunoia1729 Jun 7, 2022 • edited

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

Eunoia1729 Jun 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra commented Jun 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding left a comment

Choose a reason for hiding this comment

Eunoia1729 commented Jun 8, 2022 • edited

sanket1729 left a comment

Choose a reason for hiding this comment

tcharding left a comment

Choose a reason for hiding this comment

tcharding commented Jun 8, 2022

apoelstra left a comment • edited

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

Eunoia1729 commented Jun 5, 2022 •

edited

Eunoia1729 Jun 7, 2022 •

edited

Eunoia1729 Jun 7, 2022 •

edited

Eunoia1729 commented Jun 8, 2022 •

edited

apoelstra left a comment •

edited