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.

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

Builder functions to generate standard p2* scriptPubkey's #387

Open
wants to merge 2 commits into
base: master
from

Conversation

@dr-orlovsky
Copy link
Contributor

dr-orlovsky commented Jan 10, 2020

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
@dr-orlovsky dr-orlovsky changed the title p2* generators moved to Builder Builder functions to generate standard p2* scriptPubkey's Jan 10, 2020
@dr-orlovsky dr-orlovsky force-pushed the pandoracore:builder-p2wildcard branch 2 times, most recently from 95f3325 to 92aedb0 Jan 10, 2020
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 12, 2020

Codecov Report

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

Impacted file tree graph

@@            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.

Copy link
Collaborator

stevenroose left a comment

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 dr-orlovsky force-pushed the pandoracore:builder-p2wildcard branch from 92aedb0 to 5e205ac Jan 23, 2020
@dr-orlovsky dr-orlovsky force-pushed the pandoracore:builder-p2wildcard branch from 5e205ac to 3266c6f Jan 23, 2020
@dr-orlovsky

This comment has been minimized.

Copy link
Contributor Author

dr-orlovsky commented Jan 23, 2020

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

Copy link
Collaborator

stevenroose left a comment

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.

}

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

This comment has been minimized.

Copy link
@stevenroose

stevenroose Jan 23, 2020

Collaborator

gen_witness_program I think is a better name. You're not generating a witness, witnesses are inputs.

/// Generates P2WSH-type of scriptPubkey with a given hash of the redeem script
pub fn gen_witness(ver: bech32::u5, program: &Vec<u8>) -> Self {
assert!(ver.to_u8() <= 16);
let mut verop = ver.to_u8();

This comment has been minimized.

Copy link
@stevenroose

stevenroose Jan 23, 2020

Collaborator

The assert can be below this line to avoid calling to_u8 twice.


/// Generates P2WPKH-type of scriptPubkey
pub fn gen_v0_p2wpkh(pubkey_hash: &WPubkeyHash) -> Self {
Self::gen_witness(bech32::u5::try_from_u8(0).unwrap(), &pubkey_hash.to_vec())

This comment has been minimized.

Copy link
@stevenroose

stevenroose Jan 23, 2020

Collaborator

Please don't use Self here. Just use Builder.

@@ -200,31 +198,17 @@ impl Payload {
}

/// Generates a script pubkey spending to this [Payload].
pub fn script_pubkey(&self) -> script::Script {
pub fn script_pubkey(&self) -> script::Builder {

This comment has been minimized.

Copy link
@stevenroose

stevenroose Jan 23, 2020

Collaborator

I'd like to keep this return type a Script.

Address {
network: network,
payload: Payload::ScriptHash(ScriptHash::hash(&script[..])),

This comment has been minimized.

Copy link
@stevenroose

stevenroose Jan 23, 2020

Collaborator

Well... In this situation, the script slice already exists, so there is no advantage of using the engine...

Address {
network: network,
payload: Payload::ScriptHash(ScriptHash::hash(builder.into_script().as_bytes())),

This comment has been minimized.

Copy link
@stevenroose

stevenroose Jan 23, 2020

Collaborator

Idem here. The builder has the byte slice internally, so into_script() just returns that slice as a script and as_bytes looks into the slice. So no allocation is prevented by using the engine here either.

Address {
network: network,
payload: Payload::WitnessProgram {
version: bech32::u5::try_from_u8(0).expect("0<32"),
program: WScriptHash::hash(&script[..])[..].to_vec(),

This comment has been minimized.

Copy link
@stevenroose

stevenroose Jan 23, 2020

Collaborator

Idem.

let ws = script::Builder::new()
.push_int(0)
.push_slice(&WScriptHash::hash(&script[..])[..])

This comment has been minimized.

Copy link
@stevenroose

stevenroose Jan 23, 2020

Collaborator

Idem.

Address {
network: network,
payload: Payload::ScriptHash(ScriptHash::hash(&ws[..])),

This comment has been minimized.

Copy link
@stevenroose

stevenroose Jan 23, 2020

Collaborator

Idem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.