/bitcoin

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix and add test for empty chain and reorg consistency for gettxoutsetinfo. #10445

Merged
merged 2 commits into from May 26, 2017
+30 −2

Conversation

Reviewers

@jnewbery jnewbery

@jonasschnelli jonasschnelli

Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
7 participants
@gmaxwell @sipa @paveljanik @sdaftuar @jnewbery @jonasschnelli @fanquake
@gmaxwell
Member

gmaxwell commented May 23, 2017

No description provided.

@gmaxwell
Member

gmaxwell commented May 23, 2017 •
Edited 1 time

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.
@sipa @gmaxwell
Fix: make CCoinsViewDbCursor::Seek work for missing keys 
Thanks to Suhas Daftuar for figuring this out.
822755a

@fanquake fanquake added the Tests label May 23, 2017

@sipa
Owner

sipa commented May 24, 2017

utACK 4b3dd97
@paveljanik
Contributor

paveljanik commented May 24, 2017

ACK 4b3dd97

FWIW: The new test crashes on master with:

2017-05-24 06:50:24.034000 TestFramework (INFO): Initializing test directory /var/folders/65/fn0h49r55k7779vg1b_h461r0000gn/T/test0wr7ofxj
Assertion failed: (valid_), function key, file leveldb/db/db_iter.cc, line 67.

Fixed in this PR.
@jnewbery

jnewbery reviewed May 24, 2017
View changes

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 = 2

to:

        self.num_nodes = 1

I 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
@@ -53,6 +53,25 @@ def _test_gettxoutsetinfo(self):
assert_equal(len(res['bestblock']), 64)
assert_equal(len(res['hash_serialized']), 64)
+ b1hash = node.getblockhash(1)
@jnewbery

jnewbery May 24, 2017

Member

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")
@gmaxwell

gmaxwell May 25, 2017

Member

done

test/functional/blockchain.py
+ assert_equal(res2['transactions'], 0)
+ assert_equal(res2['total_amount'], Decimal('0'))
+ assert_equal(res2['height'], 0)
+ assert_equal(res2['txouts'], 0)
@jnewbery

jnewbery May 24, 2017

Member

Why not test all fields:

assert_equal(res2['bestblock'], node.getblockhash(0)))
assert_equal(len(res2['hash_serialized']), 64)
@gmaxwell

gmaxwell May 25, 2017

Member

done (and also added that to the first check)

test/functional/blockchain.py
+ assert_equal(res2['txouts'], 0)
+
+ node.reconsiderblock(b1hash)
+
@jnewbery

jnewbery May 24, 2017

Member

Again, a comment/info log would be nice here:

self.log.info("Test that gettxoutsetinfo() returns the same result after invalidate/reconsider block")
@gmaxwell

gmaxwell May 25, 2017

Member

done

test/functional/blockchain.py
+ node.reconsiderblock(b1hash)
+
+ res3 = node.gettxoutsetinfo()
+ assert_equal(res['total_amount'], res3['total_amount'])
@jnewbery

jnewbery May 24, 2017

Member

Consider just testing equality of res and res3 for succinctness:

assert_equal(res, res3)
@gmaxwell

gmaxwell May 25, 2017

Member

Equality will be the wrong comparison with the disk usage field which is about to be added.

@sdaftuar
Member

sdaftuar commented May 24, 2017

ACK
@gmaxwell
Member

gmaxwell commented May 25, 2017

updated for @jnewbery 's nits. (Thanks!)
@jnewbery
Member

jnewbery commented May 25, 2017

tested reACK 513da90
@sdaftuar
Member

sdaftuar commented May 25, 2017

utACK 513da90
@paveljanik
Contributor

paveljanik commented May 26, 2017

reACK 513da90

@sipa sipa merged commit 513da90 into bitcoin:master May 26, 2017
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

sipa added a commit that referenced this pull request May 26, 2017

@sipa
Merge #10445: Add test for empty chain and reorg consistency for gett… 
…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
This commit was signed with a verified signature.
@sipa sipa Pieter Wuille
GPG key ID: A636E97631F767E0 Learn about signing commits
b4b057a

@sipa sipa referenced this pull request May 26, 2017

Merged

Switch chainstate db and cache to per-txout model #10195

@gmaxwell gmaxwell changed the title from Add test for empty chain and reorg consistency for gettxoutsetinfo. to Fix and add test for empty chain and reorg consistency for gettxoutsetinfo. Jun 1, 2017

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 3, 2017

@sipa @luke-jr
Fix: make CCoinsViewDbCursor::Seek work for missing keys 
Thanks to Suhas Daftuar for figuring this out.

Github-Pull: #10445
Rebased-From: 822755a
2a02cb9

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 3, 2017

@gmaxwell @luke-jr
Add test for empty chain and reorg consistency for gettxoutsetinfo. 
Github-Pull: #10445
Rebased-From: 513da90
471f921

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 3, 2017

@sipa @luke-jr
Fix: make CCoinsViewDbCursor::Seek work for missing keys 
Thanks to Suhas Daftuar for figuring this out.

Github-Pull: #10445
Rebased-From: 822755a
41d92ff

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 3, 2017

@sipa @luke-jr
Fix: make CCoinsViewDbCursor::Seek work for missing keys 
Thanks to Suhas Daftuar for figuring this out.

Github-Pull: #10445
Rebased-From: 822755a
0bd7fc9

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 3, 2017

@sipa @luke-jr
Fix: make CCoinsViewDbCursor::Seek work for missing keys 
Thanks to Suhas Daftuar for figuring this out.

Github-Pull: #10445
Rebased-From: 822755a
01c0c34

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 5, 2017

@sipa @luke-jr
Fix: make CCoinsViewDbCursor::Seek work for missing keys 
Thanks to Suhas Daftuar for figuring this out.

Github-Pull: #10445
Rebased-From: 822755a
00230ce

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 5, 2017

@sipa @luke-jr
Fix: make CCoinsViewDbCursor::Seek work for missing keys 
Thanks to Suhas Daftuar for figuring this out.

Github-Pull: #10445
Rebased-From: 822755a
b43ac53

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 5, 2017

@sipa @luke-jr
Fix: make CCoinsViewDbCursor::Seek work for missing keys 
Thanks to Suhas Daftuar for figuring this out.

Github-Pull: #10445
Rebased-From: 822755a
87a21d5

nomnombtc added a commit to nomnombtc/bitcoin that referenced this pull request Jul 17, 2017

@sipa @nomnombtc
Fix: make CCoinsViewDbCursor::Seek work for missing keys 
Thanks to Suhas Daftuar for figuring this out.

Github-Pull: #10445
Rebased-From: 822755a
0dfd00d

codablock added a commit to codablock/dash that referenced this pull request Sep 27, 2017

@sipa @codablock
Merge #10445: Add test for empty chain and reorg consistency for gett… 
…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
22a92ff

@sickpig sickpig referenced this pull request in BitcoinUnlimited/BitcoinUnlimited Oct 1, 2017

Merged

dbcache phase 3 enhancements #785

codablock added a commit to codablock/dash that referenced this pull request Oct 12, 2017

@sipa @codablock
Merge #10445: Add test for empty chain and reorg consistency for gett… 
…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
939cf65

codablock added a commit to codablock/dash that referenced this pull request Oct 24, 2017

@sipa @codablock
Merge #10445: Add test for empty chain and reorg consistency for gett… 
…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
fc5ced3

runn1ng added a commit to runn1ng/bitcoin that referenced this pull request Oct 30, 2017

@sipa @runn1ng
Fix: make CCoinsViewDbCursor::Seek work for missing keys 
Thanks to Suhas Daftuar for figuring this out.

Github-Pull: #10445
Rebased-From: 822755a
d28ca45

UdjinM6 added a commit to UdjinM6/dash that referenced this pull request Nov 8, 2017

@sipa @UdjinM6
Merge #10445: Add test for empty chain and reorg consistency for gett… 
…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
ab4511f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment