[Wallet] Various transaction handling improvements #970
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Seeing the abandontransaction changes: Does this invalidate #825 completely, or just trigger some changes in that PR? |
|
looking quite nice, needed functionality 👌 |
|
needs rebase |
Warrows
force-pushed
the
2019_bitcoin_bp_9311
branch
from
4fd43c5
to
e17a9ec
Compare
|
Rebased |
Warrows
force-pushed
the
2019_bitcoin_bp_9311
branch
from
e17a9ec
to
4f42ac2
Compare
Warrows
force-pushed
the
2019_bitcoin_bp_9311
branch
from
4f42ac2
to
42e3604
Compare
|
Rebased, fixed PIVX specific transactions conflict detection and CMake travis job. |
Backport of bitcoin#6550 Assume that when a wallet transaction has a valid block hash and transaction position in it, the transaction is actually there. We're already trusting wallet data in a much more fundamental way anyway. To prevent backward compatibility issues, a new record is used for storing the block locator in the wallet. Old wallets will see a wallet file synchronized up to the genesis block, and rescan automatically.
Backport of bitcoin#6508 This switches the Merkle tree logic for blocks to one that runs in constant (small) space. The old code is moved to tests, and a new test is added that for various combinations of block sizes, transaction positions to compute a branch for, and mutations: * Verifies that the old code and new code agree for the Merkle root. * Verifies that the old code and new code agree for the Merkle branch. * Verifies that the computed Merkle branch is valid. * Verifies that mutations don't change the Merkle root. * Verifies that mutations are correctly detected.
Backport of bitcoin#4805 ( commit 44bc988 )
mempool Backport of bitcoin#7105 (9ac63d6)
Unconfirmed transactions that are not in your mempool either due to eviction or other means may be unlikely to be mined. abandontransaction gives the wallet a way to no longer consider as spent the coins that are inputs to such a transaction. All dependent transactions in the wallet will also be marked as abandoned.
During startup, when adding pending wallet transactions, which spend outputs of other pending wallet transactions, back to the memory pool, and when they are added out of order, it appears as if they are orphans with missing inputs. Those transactions are then rejected and flagged as "conflicting" (= not in the memory pool, not in the block chain). To prevent this, transactions are explicitly sorted.
No longer consider coins which aren't in our mempool. Add test for regression in abandonconflict.py
Prior to this change, it would mark only the first layer of child transactions abandoned, due to always following the input hashTx rather than the current now tx.
Coinbase and zerocoin transaction can't really be checked for conflicts. Coinbase has no value anyway. Zerocoin transactions are checked for zero knowledge proof, the input hash has no meaning.
Warrows
force-pushed
the
2019_bitcoin_bp_9311
branch
from
4eaa40e
to
91b48c7
Compare
Fuzzbawls
approved these changes
Oct 9, 2019
random-zebra
added a commit
that referenced
this pull request
Oct 9, 2019
91b48c7 [Build] Add new merkle files to CMake lists (warrows) 48a3aff [Wallet] Ignore coinbase and zc tx "conflicts" (warrows) 3572354 [Wallet] Fix an error in tx depth computation (warrows) 6928369 [Tests] Enable abandonconflict functional test (warrows) 34cd496 Fix that CWallet::AbandonTransaction would only traverse one level (Ben Woosley) aba5b75 Fix calculation of balances and available coins. (Alex Morcos) 48d705f Remove stale wallet transactions on initial load. (presstab) 12985ae Flush wallet after abandontransaction (Alex Morcos) 8f87956 [Wallet] sort pending wallet transactions before reaccepting (dexX7) 9c2f445 [Wallet] Call notification signal when a transaction is abandoned (Jonas Schnelli) 778ebf3 Add new rpc call: abandontransaction (Alex Morcos) 0e86c3e Make wallet descendant searching more efficient (Alex Morcos) d0083a8 Make sure conflicted wallet tx's update balances (Alex Morcos) 6a50e03 [Wallet] Keep track of explicit wallet conflicts instead of using mempool (warrows) 7ccb2b5 [Wallet] Do not flush the wallet in AddToWalletIfInvolvingMe(..) (warrows) 47345be [Refactor] Move wallet functions out of header (warrows) ab9efb8 [Wallet] Switch to a constant-space Merkle root/branch algorithm (warrows) 5447622 [Wallet] Do not store Merkle branches in the wallet (warrows) Pull request description: This pull request is a happy melting pot of improvements regarding transactions handling. Most of them are backports from bitcoin. I advise reviewers to check the code of the different commits independently to understand them more easily. However, testing is probably better done all at once. I am making a single pull request because these changes are all entangled and introducing some of them without others would probably introduce temporary bugs. ## Commits details ## - 6c3e2ac backport of bitcoin#6550 - 5304fdf backport of bitcoin#6508 - c3eeeac simple code move from the header to the cpp file. It contains no functional change. - 6cc4d37 backport of bitcoin#4805 - 10be1db backport of bitcoin#7105 - 8a34c32 backport of bitcoin#7306 - 3caf123, 9e17178 and 240f5b4 are the backport for bitcoin#7312 - ad6d0b1 backport of bitcoin#5511 - fcc07c3 backport of bitcoin#9311 - 5ed5e26 is an update of #825 - 392d504 backport of bitcoin#7715 - 7199f3a backport of bitcoin#13652 - f09d999 enables and fixes the test from bitcoin#7312 - 4fd43c5 fixes an oversight in bitcoin#7105 backport ACKs for top commit: random-zebra: ACK 91b48c7 Fuzzbawls: ACK 91b48c7 Tree-SHA512: 2628cebe98805b8048b920b51ee26fd4f0c53643d78da9b8cb265aede52dfe1d40c8c19d34293c232c5c35be7f1ab89ff5b4a07073a4b27c371ea70eb8708669
random-zebra
added a commit
to random-zebra/PIVX
that referenced
this pull request
Oct 11, 2019
Define CwalletTx::IsOrphan to handle the old (prior to PIVX-Project#970) -1 value for nDepth.
random-zebra
added a commit
to random-zebra/PIVX
that referenced
this pull request
Oct 11, 2019
Define CwalletTx::IsOrphan to handle the old (prior to PIVX-Project#970) -1 value for nDepth.
random-zebra
added a commit
that referenced
this pull request
Oct 18, 2019
…uts to the wallet 3a7ec7c [Tests] Add wallet_reorg-stake functional test to test_runner.py (random-zebra) a0285e4 [Wallet] Fix bug with coinstake inputs wrongly marked as spent (random-zebra) bb683c7 [Tests] Add wallet_reorg-stake functional test (random-zebra) 3eca8da [Core][Tests] REGTEST: fix nStakeModifier=0 (random-zebra) Pull request description: Additional bug introduced with the changes of #970 (and not caught in #1040 even though the culprit is the same: `GetDepthInMainChain` returning a value `0` when `-1` was expected). After a block reorganization, the coins used as coinstake inputs in the orphan chain were still marked as spent in the wallet. Thus there were inconsistencies in the balance (either displayed in the GUI or returned by `getbalance` via CLI) and missing utxos in the wallet (either accessed through coincontrol in the GUI or returned by `listunspent` via CLI). a0285e4 fixes it by marking as "spent" the inputs of not-in-mempool txes only for non-coinstakes (coinstakes don't hit the mempool so, when not in chain, their inputs should be considered unspent). bb683c7 Introduces the functional test `wallet_reorg-stake` to reproduce the issue. The test fails without a0285e4 and passes with it. ACKs for top commit: Warrows: ACK 3a7ec7c Tree-SHA512: 8f97e3d48720b776c84820e0ab8257665ac4c4c9d394db0e4b9f3a05b0904bf9f70cf54865338c4e29066e6b3670e9e90f77780d3ddd198e8e2b6e416c4cb49c
random-zebra
added a commit
to random-zebra/PIVX
that referenced
this pull request
Jul 7, 2020
Check for explicit wallet conflicts when fFromLoadWallet (from bitcoin/bitcoin@9ac63d6, wrongly backported in PIVX-Project#970)
furszy
added a commit
that referenced
this pull request
Jul 8, 2020
…et::AddToWallet 1839ec7 [BUG] fix detection of wallet conflicts in CWallet::AddToWallet (random-zebra) Pull request description: Check for explicit wallet conflicts when `fFromLoadWallet` (from bitcoin/bitcoin@9ac63d6, wrongly backported in #970). ACKs for top commit: furszy: Nice find!, ACK 1839ec7 Fuzzbawls: utACK 1839ec7 Tree-SHA512: 78dfd111fc7f5c681758a01b24921c2cc882e801c0fc5375d274d96a5ec0f62491d77ef725b560726f05cf68e16f0f8a5dc30a3c9ed8442ed5294c15bca069ce
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request is a happy melting pot of improvements regarding transactions handling. Most of them are backports from bitcoin. I advise reviewers to check the code of the different commits independently to understand them more easily. However, testing is probably better done all at once.
I am making a single pull request because these changes are all entangled and introducing some of them without others would probably introduce temporary bugs.
Commits details