Fix and add test for empty chain and reorg consistency for gettxoutsetinfo. #10445
Conversation
|
This will crash on master. Fix is currently in the per-txout PR. Edit: Sipa asked me to just add the fix here, doing so now. |
Thanks to Suhas Daftuar for figuring this out.
|
utACK 4b3dd975a303a0d7efaec790c387f0285a4bb8c4 |
jonasschnelli
left a comment
jonasschnelli
left a comment
There was a problem hiding this comment.
utACK 4b3dd975a303a0d7efaec790c387f0285a4bb8c4
|
ACK 4b3dd97 FWIW: The new test crashes on master with: Fixed in this PR. |
jnewbery
left a comment
jnewbery
left a comment
There was a problem hiding this comment.
I can't review the code change in src/txdb.cpp since I don't know that code, but I can confirm that this test causes bitcoind to crash without the code change, and passes successfully with the code change.
A few nits inline. Also consider changing line 35 of the test from:
self.num_nodes = 2to:
self.num_nodes = 1I know that wsan't changed by this PR, but 2 nodes are unnecessary for this test, and changing it to 1 cuts the test time in half (from 5s to 2.5s on my pc) due to node startup/shutdown time.
test/functional/blockchain.py
Outdated
There was a problem hiding this comment.
Can you add a comment (or info log) here explaining what this test is for:
self.log.info("Test that gettxoutsetinfo() works for blockchain with just the genesis block")
test/functional/blockchain.py
Outdated
There was a problem hiding this comment.
Why not test all fields:
assert_equal(res2['bestblock'], node.getblockhash(0)))
assert_equal(len(res2['hash_serialized']), 64)There was a problem hiding this comment.
done (and also added that to the first check)
test/functional/blockchain.py
Outdated
There was a problem hiding this comment.
Again, a comment/info log would be nice here:
self.log.info("Test that gettxoutsetinfo() returns the same result after invalidate/reconsider block")
test/functional/blockchain.py
Outdated
There was a problem hiding this comment.
Consider just testing equality of res and res3 for succinctness:
assert_equal(res, res3)There was a problem hiding this comment.
Equality will be the wrong comparison with the disk usage field which is about to be added.
|
ACK |
|
updated for @jnewbery 's nits. (Thanks!) |
|
tested reACK 513da90 |
|
utACK 513da90 |
|
reACK 513da90 |
…xoutsetinfo. 513da90 Add test for empty chain and reorg consistency for gettxoutsetinfo. (Gregory Maxwell) 822755a Fix: make CCoinsViewDbCursor::Seek work for missing keys (Pieter Wuille) Tree-SHA512: e549921e8b8f599bf61ebe0ee7ef1d2f474043723d633e24665fe434b996a98e039612de8a1c2cd16b63f154943ff5ea1c1935e9561cfb813a00d47d926d0b22
gmaxwell
changed the title
Thanks to Suhas Daftuar for figuring this out. Github-Pull: bitcoin#10445 Rebased-From: 822755a
…or gettxoutsetinfo. 513da90 Add test for empty chain and reorg consistency for gettxoutsetinfo. (Gregory Maxwell) 822755a Fix: make CCoinsViewDbCursor::Seek work for missing keys (Pieter Wuille) Tree-SHA512: e549921e8b8f599bf61ebe0ee7ef1d2f474043723d633e24665fe434b996a98e039612de8a1c2cd16b63f154943ff5ea1c1935e9561cfb813a00d47d926d0b22
7 participants
No description provided.