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

Add test for CWalletTx::GetImmatureCredit() returning stale values. #9359

Merged
merged 1 commit into from Mar 2, 2017

Conversation

Projects
None yet
5 participants
@ryanofsky
Contributor

ryanofsky commented Dec 16, 2016

Fix CWalletTx::GetImmatureCredit() returning stale values.

One line fix. Add missing assignment to CWalletTx::MarkDirty() to clear the cached immature credit flag.

Updated description: Add test for cached immature credit flag not being cleared in CWalletTx::MarkDirty() bug, which was fixed in #8717, commit a560378.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Dec 16, 2016

Member

Seems a duplicate of #8717?

(except that you add a test)

Member

laanwj commented Dec 16, 2016

Seems a duplicate of #8717?

(except that you add a test)

@fanquake fanquake added the Wallet label Dec 16, 2016

@ryanofsky

This comment has been minimized.

Show comment
Hide comment
@ryanofsky

ryanofsky Dec 16, 2016

Contributor

@laanwj, it's basically the same as #8717 (which I didn't know about) except it adds the test and places the new assignment in between fCreditCached and fAvailableCreditCached following the order of CWalletTx class members. So I think it's a slightly better PR.

Contributor

ryanofsky commented Dec 16, 2016

@laanwj, it's basically the same as #8717 (which I didn't know about) except it adds the test and places the new assignment in between fCreditCached and fAvailableCreditCached following the order of CWalletTx class members. So I think it's a slightly better PR.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Dec 21, 2016

Member

Right. Will merge #8717 first, then we can rebase this into a test-only pull.
(utACK otherwise)

Member

laanwj commented Dec 21, 2016

Right. Will merge #8717 first, then we can rebase this into a test-only pull.
(utACK otherwise)

@MarcoFalke MarcoFalke added the Tests label Dec 21, 2016

// Call GetImmatureCredit() once before adding the key to the wallet to
// cache the current immature credit amount, which is 0.
BOOST_CHECK_EQUAL(wtx.GetImmatureCredit(), 0);

This comment has been minimized.

@MarcoFalke

MarcoFalke Dec 21, 2016

Member

Travis:

Assertion failed: lock cs_main not held in ../../src/wallet/wallet.cpp:3761; locks held:
Running 221 test cases...
unknown location(0): fatal error: in "wallet_tests/coin_mark_dirty_immature_credit": signal: SIGABRT (application abort requested)
../../src/wallet/test/wallet_tests.cpp(371): last checkpoint
test_bitcoin: ../../src/key.cpp:300: void ECC_Start(): Assertion `secp256k1_context_sign == __null' failed.
unknown location(0): fatal error: in "wallet_tests/coin_selection_tests": signal: SIGABRT (application abort requested)
../../src/wallet/test/wallet_tests.cpp(70): last checkpoint: "coin_selection_tests" fixture entry.
test_bitcoin: ../../src/key.cpp:300: void ECC_Start(): Assertion `secp256k1_context_sign == __null' failed.
unknown location(0): fatal error: in "wallet_crypto/decrypt": signal: SIGABRT (application abort requested)
../../src/wallet/test/crypto_tests.cpp(217): last checkpoint: "decrypt" fixture entry.
@MarcoFalke

MarcoFalke Dec 21, 2016

Member

Travis:

Assertion failed: lock cs_main not held in ../../src/wallet/wallet.cpp:3761; locks held:
Running 221 test cases...
unknown location(0): fatal error: in "wallet_tests/coin_mark_dirty_immature_credit": signal: SIGABRT (application abort requested)
../../src/wallet/test/wallet_tests.cpp(371): last checkpoint
test_bitcoin: ../../src/key.cpp:300: void ECC_Start(): Assertion `secp256k1_context_sign == __null' failed.
unknown location(0): fatal error: in "wallet_tests/coin_selection_tests": signal: SIGABRT (application abort requested)
../../src/wallet/test/wallet_tests.cpp(70): last checkpoint: "coin_selection_tests" fixture entry.
test_bitcoin: ../../src/key.cpp:300: void ECC_Start(): Assertion `secp256k1_context_sign == __null' failed.
unknown location(0): fatal error: in "wallet_crypto/decrypt": signal: SIGABRT (application abort requested)
../../src/wallet/test/crypto_tests.cpp(217): last checkpoint: "decrypt" fixture entry.

This comment has been minimized.

@ryanofsky

ryanofsky Jan 4, 2017

Contributor

Thanks, the locking error has been fixed.

@ryanofsky

ryanofsky Jan 4, 2017

Contributor

Thanks, the locking error has been fixed.

@ryanofsky ryanofsky changed the title from Fix CWalletTx::GetImmatureCredit() returning stale values. to Add test for CWalletTx::GetImmatureCredit() returning stale values. Dec 21, 2016

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Feb 9, 2017

Member

ACK 52045d1 (test passes on version rebased onto master)

Member

kallewoof commented Feb 9, 2017

ACK 52045d1 (test passes on version rebased onto master)

Add test for CWalletTx::GetImmatureCredit() returning stale values.
Add test for cached immature credit flag not being cleared in
CWalletTx::MarkDirty() bug, which was fixed in
#8717, commit a560378.
@ryanofsky

This comment has been minimized.

Show comment
Hide comment
@ryanofsky

ryanofsky Mar 1, 2017

Contributor

Rebased 52045d1 -> 7ed143c (immature.5 -> immature.6) because of conflict with recent changes in wallet_tests.cpp.

Contributor

ryanofsky commented Mar 1, 2017

Rebased 52045d1 -> 7ed143c (immature.5 -> immature.6) because of conflict with recent changes in wallet_tests.cpp.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Mar 2, 2017

Member

utACK 7ed143c. Sorry, somehow forgot about this pull.

Member

MarcoFalke commented Mar 2, 2017

utACK 7ed143c. Sorry, somehow forgot about this pull.

@MarcoFalke MarcoFalke merged commit 7ed143c into bitcoin:master Mar 2, 2017

1 check passed

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

MarcoFalke added a commit that referenced this pull request Mar 2, 2017

Merge #9359: Add test for CWalletTx::GetImmatureCredit() returning st…
…ale values.

7ed143c Add test for CWalletTx::GetImmatureCredit() returning stale values. (Russell Yanofsky)

Tree-SHA512: c95088ed6dfc5a0774ddaa2fe14ac0a9ebd830922a4d77100ec3d51fdeb6df40ad97de4f2ea970ed0f4122dcc0022ee1d43ab3c7188becd7f90c1c6af0ed39b7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment