Implement Async Script Verification RFC #961
Conversation
zebra-consensus/src/script.rs
Outdated
| }) | ||
| .flat_map(|inputs| inputs.into_iter()); | ||
|
|
||
| assert!(inputs.any(|input| matches!(input, transparent::Input::PrevOut { .. }))); |
There was a problem hiding this comment.
I tried writing a test but it ended up being perfectly, beautifully useless. This assert fails. Not really sure how to test this one...
There was a problem hiding this comment.
The first block with a non-coinbase transaction is at least block 101, because of the coinbase maturity rule.
Should we focus on getting the sync acceptance tests working? (#1141)
If we only want to test finalized state, then we could add a sync test for the second checkpoint, which has 2000 blocks.
There was a problem hiding this comment.
I think that might be necessary, but even then I'm not sure we can test this code meaningfully via the acceptance test, given the restriction only being able to chain verify post sapling blocks.
zebra-consensus/src/script.rs
Outdated
| .boxed() | ||
| } | ||
| transparent::Input::Coinbase { .. } => unimplemented!( | ||
| "how should we handle verifying coinbase transactions in the script::Verifier?" |
There was a problem hiding this comment.
The block height MUST be encoded as the first item in the coinbase transaction’s scriptSig, as specified in [BIP-34]. The format of the height is “serialized CScript” – the first byte is the number of bytes in the number, and the following bytes are the signed little-endian representation of the number.
There was a problem hiding this comment.
I think the async script verifier should check the script part of this rule, and then the subsidy module should check the address:
Consensus rule: [Pre-Canopy] A coinbase transaction at height ∈ {1..FoundersRewardLastBlockHeight} MUST include at least one output that pays exactly FoundersReward(height) zatoshi with a standard P2SH script of the form OP_HASH160 FounderRedeemScriptHash(height) OP_EQUAL as its scriptPubKey.
There was a problem hiding this comment.
Similarly with transparent founders rewards:
Consensus rule: [Canopy onward] The coinbase transaction at block height
heightMUST contain at least one output per funding streamfsactive atheight, that paysfs.Value(height)zatoshi in the prescribed way to the stream’s recipient address represented byfs.AddressListfs.AddressIndex(height).
The “prescribed way” to pay a transparent P2SH address is to use a standard P2SH script of the form OP_HASH160fs.RedeemScriptHash(height)OP_EQUAL as the scriptPubKey. Herefs.RedeemScriptHash(height)is the standard redeem script hash for the recipient address given byfs.AddressList[fs.AddressIndex(height)]in Base58Check form. The standard redeem script hash is specified in [Bitcoin-Multisig] for P2SH multisig addresses, or [Bitcoin-P2SH] for other P2SH addresses.
There was a problem hiding this comment.
To be clear, I think we can merge this PR now, and open a ticket for coinbase script validation.
There was a problem hiding this comment.
sounds good, tysm for looking this up, I'll create the issue rn and vendor this information there.
There was a problem hiding this comment.
I don't think that this is the appropriate place to check this rule, because this rule isn't actually a rule scoped to a particular script input, but a rule scoped to a block: each block has exactly one transaction with coinbase inputs, of such and such form, etc.
So, I think that a better strategy that keeps the script verifier scoped to script verification would be to handle the special coinbase transaction specially, and reject coinbase inputs in the script verifier (which would then be scoped to ordinary transactions).
| .zcash_serialize_to_vec() | ||
| .expect("zcash_serialize_to_vec has wrong return type"); | ||
|
|
||
| // TODO: check highest entry of hash_by_height as in RFC |
There was a problem hiding this comment.
Is there a ticket or checklist item for this TODO?
There was a problem hiding this comment.
no idea, the comment predates this PR
There was a problem hiding this comment.
I wrote that comment when implementing the most basic version of the sled state that would unblock the sync work. It should be on the list of things to implement to complete the sled state, along with the other TODOs in the source code below.
zebra-consensus/src/script.rs
Outdated
| #[spandoc::spandoc] | ||
| async fn happy_path_test() -> Result<(), Report> { |
There was a problem hiding this comment.
It seems like this test is actually 3-4 separate tests in the same function.
Would it help to split them up?
There was a problem hiding this comment.
its two tests, tho the second one (the initial assert) was added because I realized the rest of the test was useless and I wanted to write a short gist of why this test was doomed to fail. I don't think splitting it up is useful, I still have to read the other comments, so there might be a suggestion for how to make this actually useful that I've missed but my expectation is that this entire test needs to get tossed in the trash.
Co-authored-by: teor <teor@riseup.net>
hdevalence
dismissed
teor2345’s stale review
The behavior we need to test here is the integration, not the verification, so we need to be able to integrate it before we can test it.
blocked on the implementation of RFC5