test(consensus): test BlockTooBig#867
Conversation
JoseSK999
added
chore
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
|
Sometimes github is strange, it duplicated my review and now theres only one... The one that I marked as duplicate. |
JoseSK999
force-pushed
the
block-too-big-test
branch
from
39bec94 to
dc543b1
Compare
|
ACK dc543b1 |
| let mut it = coinbase.input[0].witness.iter(); | ||
| match (it.next(), it.next()) { | ||
| (Some(rv), None) => rv.try_into().ok()?, | ||
| _ => return None, |
There was a problem hiding this comment.
Nit: since the coinbase input must have one and only one element, what about leaving an unreachable here?
There was a problem hiding this comment.
Makes sense
Actually better to return None, this may be a legacy block, so unreachable is not true.
| fn decode_block(file_path: &str) -> Block { | ||
| let block_file = File::open(file_path).unwrap(); | ||
| let block_bytes = zstd::decode_all(block_file).unwrap(); | ||
| deserialize(&block_bytes).unwrap() |
There was a problem hiding this comment.
Unrelated: Bitcoin Core leaves those test assets in another repo, called qa-assets. Leaving in the same tee has the disadvantage of bloating our tree with those test data, even though some people may not use it. Wdyt about moving these to our own floresta-qa-assets?
There was a problem hiding this comment.
@Davidson-Souza you mean the data under floresta-chain/testdata?
There was a problem hiding this comment.
The idea that i got was is that davidson wants a floresta-qa-assets to gather test utils/data
There was a problem hiding this comment.
oh like a separate test utils crate
JoseSK999
force-pushed
the
block-too-big-test
branch
from
dc543b1 to
20eb38b
Compare
|
Pushed 20eb38b
|
I've constructed a transaction that, when inserted in block 866,342 makes it have 4,000,001 WUs. Then we update the witness commitment and the merkle root, and then check we get `BlockTooBig`. For this I had to implement the `update_witness_commitment` helper. Note that the block we are testing has witnesses, so the witness commitment is enforced by consensus, and inserting a legacy transaction adds a zero `wtxid` that the commitment must now account for.
JoseSK999
force-pushed
the
block-too-big-test
branch
from
20eb38b to
877ddf0
Compare
|
Clippy didn't like the childish seed, fixed |
csgui
added
reliability
| let mut rng = OsRng; | ||
| /// Modifies a block to have a different output script (txdata is tampered with). | ||
| fn mutate_block(block: &mut Block) { | ||
| let mut rng = StdRng::seed_from_u64(0x_bebe_cafe); |
| /// Modifies historical block at height 866,342 by adding one extra transaction so that the | ||
| /// updated block weight is 4,000,001 WUs. The block merkle roots are updated accordingly. | ||
| fn build_oversized_866_342() -> Block { | ||
| let mut block = decode_block("./testdata/block_866342/raw.zst"); |
There was a problem hiding this comment.
Consider remove hard coded file location to avoid test failures on other machines.
csgui
left a comment
csgui
left a comment
There was a problem hiding this comment.
Left a few comments. Loved the 0xbebe_cafe
4 participants
Description and Notes
First commit adds some test helpers to make them simpler, second adds the test. I recommend review by commit, although this PR is small.
I've constructed a transaction that, when inserted in block 866,342 makes it have 4,000,001 WUs. Then we update the witness commitment and the merkle root, and then check we get
BlockTooBig.For this I had to implement the
update_witness_commitmenthelper. Note that the block we are testing has witnesses, so the witness commitment is enforced by consensus, and inserting a legacy transaction adds a zerowtxidthat the commitment must now account for.How to verify the changes you have done?
Just take a look at
test_block_too_big.