Refactoring script generating functions into a single place by dr-orlovsky · Pull Request #387 · rust-bitcoin/rust-bitcoin

dr-orlovsky · 2020-01-10T21:51:26Z

I know that we do plan to utilize miniscript as much as possible, but as an intermediate step I propose to sort out some code duplication issues. Right now, we had script generation logic in address::Payload, which was a duplication of the same code in Script class. With this PR I have:

moved all logic to Builder, which seemed to be a more appropriate location
added P2PK generation logic
added test cases
* sorted out unnecessary usage of HashEngine where hash function of the new hash types can be used instead

src/blockdata/script.rs

codecov-io · 2020-01-12T13:13:02Z

Codecov Report

Merging #387 into master will decrease coverage by 0.57%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #387      +/-   ##
==========================================
- Coverage   85.55%   84.97%   -0.58%     
==========================================
  Files          40       40              
  Lines        8660     7315    -1345     
==========================================
- Hits         7409     6216    -1193     
+ Misses       1251     1099     -152

Impacted Files	Coverage Δ
src/util/address.rs	`85.98% <100%> (-0.24%)`	⬇️
src/blockdata/script.rs	`80.29% <100%> (+0.52%)`	⬆️
src/network/message_blockdata.rs	`89.23% <0%> (-3.7%)`	⬇️
src/blockdata/block.rs	`78.83% <0%> (-1.59%)`	⬇️
src/util/contracthash.rs	`78.5% <0%> (-1.29%)`	⬇️
src/blockdata/transaction.rs	`93.92% <0%> (-0.94%)`	⬇️
src/util/hash.rs	`93.75% <0%> (-0.85%)`	⬇️
src/util/bip32.rs	`87.14% <0%> (-0.83%)`	⬇️
src/consensus/encode.rs	`85.15% <0%> (-0.44%)`	⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7587c4b...92aedb0. Read the comment docs.

stevenroose

2 major concerns:

Try to avoid allocations for generating hashes. The HashTrait::hash method is useful if the slice to be hashed already exists. Like for a Script. If the data to be hashed still needs to be created (i.e. serialized), then you can avoid allocating data by writing directly into the hash engine. Efficiency is more important than readability in for these cases.
Exactly what code was duplicated? I see that here you move the p2* creation from address.rs to script.rs, but where did this code exist a second time?

I'm a bit double as to where this code belongs. The notion of p2* scripts has to exist in address.rs, but doesn't necessarily have to exist in script.rs. So that suggests it should stay in address.rs. The only exception is p2pk for which there isn't an address.

Argh, perhaps I can agree with templating code being in Builder; it kinda makes sense. First of all please remove all unnecessary allocations.

dr-orlovsky · 2020-01-23T03:56:04Z

@stevenroose I have reverted to hash engine procedure everywhere; so pls reconsider this version for the merge

stevenroose

Now you changed some of the simpler hash functions into an engine without there being a reason for that..

Also, perhaps since the Builder::gen_* methods already construct a new builder so they can't be used on an existing builder, they should just also return a Script directly. That way they are just some kind of factory pattern. I think it's extremely uncommon to gen a template script and then add stuff to it. And it would make our code a lot more readable without all the into_scripts.

src/blockdata/script.rs

src/util/address.rs

dr-orlovsky · 2020-07-21T18:40:02Z

Sorry for doing this such late; @stevenroose everything is fixed according to your comments + rebased onto master

stevenroose · 2020-08-09T14:54:28Z

You seem to have a Rust v1.22 compilation error. I'm thinking if it actually makes sense to have the gen_xxx methods return a Builder instead of a Script directly. In the latter case, it would be a factory pattern, which might be fine for this kind of stuff. I can't see a situation where it'd be useful to have the raw builder and do something with it.

sgeisler · 2020-08-09T15:32:40Z

I'm not sure if these methods belong into the Builder at all. They produce complete scripts, there is no use case for appending opcodes after generating them and I'm not sure what would be the result of doing so (just a longer script or unwanted side effects). Imo these functions would be wonderful Script constructors, not Builder constructors.

dr-orlovsky · 2020-09-03T21:58:29Z

@sgeisler, @stevenroose I have moved all methods to Script and prefixed them with with_* (instead of gen_) according to Rust https://doc.rust-lang.org/1.0.0/style/style/naming/README.html#general-conventions-[rfc-#430].

It seems to me that @apoelstra comment from other PR #388 (comment) was actually addressing this PR (and not that one). I tend to agree that this generators are natural part of miniscript; however this code was anyway inside rust-bitcoin, so I have not written it but just refactored from scattered places across the library into a single set of functions. This may be a first step for moving this functions out/into miniscript.

This PR also increases test coverage of the original rust bitcoin code

…ript

apoelstra · 2020-09-10T11:50:33Z

src/blockdata/script.rs

    }

+    /// Generates OP_RETURN-type of scriptPubkey for a given data
+    pub fn with_op_return(data: &Vec<u8>) -> Self {


Please take a &[u8] rather than an &Vec<u8>.

fixed with the new commit, as well as the rest of your points

apoelstra · 2020-09-10T11:50:42Z

src/blockdata/script.rs

+    pub fn with_op_return(data: &Vec<u8>) -> Self {
+        Builder::new()
+            .push_opcode(opcodes::all::OP_RETURN)
+            .push_slice(&data)


This & is unnecessary

apoelstra · 2020-09-10T11:51:59Z

src/blockdata/script.rs

    pub fn new() -> Script { Script(vec![].into_boxed_slice()) }

+    /// Generates P2PK-type of scriptPubkey
+    pub fn with_pk(pubkey: &PublicKey) -> Script {


This should be named p2pk or something similar to indicate that you are making a pay-to-pubkey scriptpubkey. The with_ naming is confusing here.

Right. I've taken with_ prefix after Rust naming guidelines https://rust-lang.github.io/api-guidelines/naming.html suggesting to name constructors as new or with_ prefix. But I agree that this is confusing and guidelines are not a law.

Renamed all methods to corresponding descriptor names with new_ prefix to signify that we are not converting existing script object into a scriptPubkey but constructing a new one from arguments.

apoelstra · 2020-09-10T11:52:25Z

src/blockdata/script.rs

+    }
+
+    /// Generates P2PKH-type of scriptPubkey
+    pub fn with_pkh(pubkey_hash: &PubkeyHash) -> Script {


Ditto here, the name should indicate that this is a pay-to-pubkeyhash scriptpubkey

apoelstra · 2020-09-10T11:53:05Z

src/blockdata/script.rs

+    }
+
+    /// Generates P2WSH-type of scriptPubkey with a given hash of the redeem script
+    pub fn with_witness_program(ver: ::bech32::u5, program: &Vec<u8>) -> Script {


Don't take a &Vec<u8>

apoelstra · 2020-09-10T11:54:43Z

concept ACK. I like having these methods in Script; the current API makes you go through Address which is super annoying.

elichai

Looks good!
utACK

stale

dr-orlovsky commented Jan 10, 2020

View reviewed changes

src/blockdata/script.rs Outdated Show resolved Hide resolved

dr-orlovsky changed the title ~~p2* generators moved to Builder~~ Builder functions to generate standard p2* scriptPubkey's Jan 10, 2020

dr-orlovsky force-pushed the builder-p2wildcard branch 2 times, most recently from 95f3325 to 92aedb0 Compare January 12, 2020 12:59

stevenroose requested changes Jan 13, 2020

View reviewed changes

dr-orlovsky force-pushed the builder-p2wildcard branch 2 times, most recently from 5e205ac to 3266c6f Compare January 23, 2020 03:54

stevenroose previously requested changes Jan 23, 2020

View reviewed changes

dr-orlovsky force-pushed the builder-p2wildcard branch from 3266c6f to c110dbc Compare July 21, 2020 18:39

dr-orlovsky requested a review from stevenroose July 21, 2020 18:39

dr-orlovsky force-pushed the builder-p2wildcard branch 2 times, most recently from df952d4 to 305a252 Compare July 21, 2020 18:51

dr-orlovsky force-pushed the builder-p2wildcard branch 3 times, most recently from 07c4976 to 4221aa6 Compare September 3, 2020 21:55

dr-orlovsky changed the title ~~Builder functions to generate standard p2* scriptPubkey's~~ Refactoring script generating functions into a single place Sep 3, 2020

This was referenced Sep 3, 2020

Hash functions for public keys and scripts #388

Merged

Added hash Preimages to psbt #465

Merged

dr-orlovsky added 2 commits September 10, 2020 12:09

All script generation functionality moved to standalone methods in Sc…

9143fd3

…ript

OP_RETURN script initializer

e33cdcc

dr-orlovsky force-pushed the builder-p2wildcard branch from 4221aa6 to e33cdcc Compare September 10, 2020 10:09

apoelstra reviewed Sep 10, 2020

View reviewed changes

Fixing review comments for script generators refactoring

38a412c

apoelstra approved these changes Sep 10, 2020

View reviewed changes

elichai approved these changes Sep 10, 2020

View reviewed changes

apoelstra mentioned this pull request Sep 10, 2020

descriptors: define script_code() for legacy outputs rust-bitcoin/rust-miniscript#127

Merged

apoelstra merged commit 49e97cc into rust-bitcoin:master Sep 11, 2020

apoelstra added the minor API Change This PR should get a minor version bump label Sep 11, 2020

dr-orlovsky mentioned this pull request Sep 12, 2020

Make upstream PRs accepted & publish v0.1.0 release on crates.io LNP-BP/rust-lnpbp#77

Closed

22 tasks

PastaPastaPasta pushed a commit to PastaPastaPasta/rust-dashcore that referenced this pull request Feb 2, 2026

chore: drop unused "peer sent headers2" tracking (rust-bitcoin#387)

c121a10

Conversation

dr-orlovsky commented Jan 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

codecov-io commented Jan 12, 2020

Codecov Report

Uh oh!

stevenroose left a comment

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky commented Jan 23, 2020

Uh oh!

stevenroose left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dr-orlovsky commented Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevenroose commented Aug 9, 2020

Uh oh!

sgeisler commented Aug 9, 2020

Uh oh!

dr-orlovsky commented Sep 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

apoelstra Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

apoelstra Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

apoelstra Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

apoelstra Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Sep 10, 2020

Uh oh!

elichai left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dr-orlovsky commented Jan 10, 2020 •

edited

Loading

dr-orlovsky commented Jul 21, 2020 •

edited

Loading

dr-orlovsky commented Sep 3, 2020 •

edited

Loading