Fix for listsinceblock not filtering conflicted transactions#10470
Closed
mchrostowski wants to merge 3 commits intobitcoin:masterfrom
Closed
Fix for listsinceblock not filtering conflicted transactions#10470mchrostowski wants to merge 3 commits intobitcoin:masterfrom
mchrostowski wants to merge 3 commits intobitcoin:masterfrom
Conversation
Tests that conflicted transactions are not returned by calls to listsinceblock that filter out the confirmed transaction with which they are conflicted.
listsinceblock now checks that returned transactions are not conflicting with any transactions that are filtered out by the given blockhash.
Contributor
|
ACK - I can reproduce the problem, and this does fix it =) |
Contributor
|
utACK df09c3a |
Contributor
|
Needs rebase though now. |
Contributor
|
@mchrostowski any plans to rebase this? |
meshcollider
added a commit
that referenced
this pull request
Nov 5, 2019
436ad43 Fix issue with conflicted mempool tx in listsinceblock (Adam Jonas) Pull request description: Closes #8752 by bringing back abandoned #10470. This now checks that returned transactions are not conflicting with any transactions that are filtered out by the given blockhash and add a functional test to prevent this in the future. For more context, #8757 was closed in favor of #10470. ACKs for top commit: instagibbs: utACK 436ad43 kallewoof: utACK 436ad43 jonatack: I'm not qualifed to give an ACK here but 436ad43 appears reasonable. Built/ran tests/verified that this test fails without the change in rpcwallet.cpp: Tree-SHA512: 63d75cd3d3f19fc84dc38899b200c96179b82b24db263cd0116ee5b715265be647157855c2e35912d2fbc49c7b37db9375d6aab0ac672f0f09bece8431de5ea9
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Nov 7, 2019
…eblock 436ad43 Fix issue with conflicted mempool tx in listsinceblock (Adam Jonas) Pull request description: Closes bitcoin#8752 by bringing back abandoned bitcoin#10470. This now checks that returned transactions are not conflicting with any transactions that are filtered out by the given blockhash and add a functional test to prevent this in the future. For more context, bitcoin#8757 was closed in favor of bitcoin#10470. ACKs for top commit: instagibbs: utACK bitcoin@436ad43 kallewoof: utACK 436ad43 jonatack: I'm not qualifed to give an ACK here but 436ad43 appears reasonable. Built/ran tests/verified that this test fails without the change in rpcwallet.cpp: Tree-SHA512: 63d75cd3d3f19fc84dc38899b200c96179b82b24db263cd0116ee5b715265be647157855c2e35912d2fbc49c7b37db9375d6aab0ac672f0f09bece8431de5ea9
sidhujag
pushed a commit
to syscoin-core/syscoin
that referenced
this pull request
Nov 10, 2020
…eblock 436ad43 Fix issue with conflicted mempool tx in listsinceblock (Adam Jonas) Pull request description: Closes bitcoin#8752 by bringing back abandoned bitcoin#10470. This now checks that returned transactions are not conflicting with any transactions that are filtered out by the given blockhash and add a functional test to prevent this in the future. For more context, bitcoin#8757 was closed in favor of bitcoin#10470. ACKs for top commit: instagibbs: utACK bitcoin@436ad43 kallewoof: utACK 436ad43 jonatack: I'm not qualifed to give an ACK here but 436ad43 appears reasonable. Built/ran tests/verified that this test fails without the change in rpcwallet.cpp: Tree-SHA512: 63d75cd3d3f19fc84dc38899b200c96179b82b24db263cd0116ee5b715265be647157855c2e35912d2fbc49c7b37db9375d6aab0ac672f0f09bece8431de5ea9
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
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.
Fix for issue #8752 which, if merged, replaces PR #8757
I prepped functional/listsinceblock.py for additional tests.
Wrote a test for the above issue.
Made a fix to rpcwallet.cpp:listsinceblock(...).
The fix takes advantage of CMerkleTx::GetDepthInMainChain() returning a negative value for conflicted transactions. The negative value represents the depth of the transaction with which it is conflicted. Taking abs(...) of this lets us use the same logic for filtering un-conflicted transactions to filter the conflicted ones.