Fix issue with conflicted mempool tx in listsinceblock #17258

adamjonas · 2019-10-25T19:08:48Z

Closes #8752 by bringing back abandoned #10470 originally written by @mchrostowski.

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.

fanquake · 2019-10-25T19:17:03Z

Was the intention to cherry-pick / attribute the original author here? You could add a Co-Authored-By: Author Name <[email protected]> to the commit body.

adamjonas · 2019-10-25T20:01:36Z

Thanks @fanquake. It was extra work to rebase original commits since listsinceblock.py was merged into wallet_listsinceblock.py. Updated to reflect the contribution of the original author.

DrahtBot · 2019-10-25T20:32:41Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15931 (Remove GetDepthInMainChain dependency on locked chain interface by ariard)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

instagibbs

Looks ok enough. Test seems to make sense too, but I'm not the guy to ask about listsinceblock.

@kallewoof I recollect you filing issues on the topic, could you tag in?

instagibbs · 2019-10-28T13:13:50Z

test/functional/wallet_listsinceblock.py

That should already be in test_framework.messages module

Right. Updated.

instagibbs · 2019-10-28T13:20:07Z

test/functional/wallet_listsinceblock.py

This was copied from the test_reorg test, but that doesn't seem necessary here, so knocked it down to 1.

listsinceblock now checks that returned transactions are not conflicting with any transactions that are filtered out by the given blockhash Co-Authored-By: Michael Chrostowski <[email protected]>

instagibbs · 2019-10-28T14:41:43Z

utACK 436ad43

but someone with more history with the RPC call should review

kallewoof · 2019-11-02T11:31:42Z

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:

File "./wallet_listsinceblock.py", line 340, in double_spends_filtered
  assert_equal(original_found, False)

Feel free to ignore the comments below.

jonatack · 2019-11-04T17:35:26Z

src/wallet/rpcwallet.cpp

        CWalletTx tx = pairWtx.second;

-        if (depth == -1 || tx.GetDepthInMainChain(*locked_chain) < depth) {
+        if (depth == -1 || abs(tx.GetDepthInMainChain(*locked_chain)) < depth) {


Perhaps document in the code here why this works. To borrow from the original author, that this 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 allows using the same logic for filtering un-conflicted transactions to filter the conflicted ones.

jonatack · 2019-11-04T17:37:55Z

test/functional/wallet_listsinceblock.py

    connect_nodes,
 )

+from decimal import Decimal


nit: IIRC the codebase convention is to place language imports above codebase ones

meshcollider · 2019-11-05T08:54:35Z

LGTM 436ad43

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

mchrostowski · 2019-11-05T18:45:44Z

I'm still confused as to why you put your name on my work.

Good job getting the fix in.

@adamjonas I see now this was not possible with a rebase and my comment was out of line. Thanks for the attribution!

kallewoof · 2019-11-06T08:27:19Z

I'm still confused as to why you put your name on my work.

Good job getting the fix in.

You didn't respond to requests on your pull request for two years, that's probably why. And your name is on the commit, it just now has another user's name as well, which is fair since they did work to get it merged after all.

laanwj · 2019-11-06T12:07:01Z

I'm still confused as to why you put your name on my work.

I don't think that complaint is warranted. Looking at the commit it's attributed in every way possible.

Fix issue with conflicted mempool tx in listsinceblock
@adamjonas
@mchrostowski
adamjonas and mchrostowski committed 16 days ago

listsinceblock now checks that returned transactions are not
conflicting with any transactions that are filtered out by
the given blockhash

Co-Authored-By: Michael Chrostowski <[email protected]>

It's also bad etiquette to change a commit and not add your own name, because it's no longer the original work.

If you disagree with this kind of use of your code, please do not contribute it under the MIT license (even implicitly by contributing it a repository licensed thus) in the first place.

ryanofsky · 2019-11-06T16:47:21Z

I don't think that complaint is warranted. Looking at the commit it's attributed in every way possible.

Basically agree, but it is possible to give credit beyond adding the original author name to metadata. You can say, "This idea was originally proposed by [person] in [url]", "This was originally implemented by [person] at [url] and the following changes have been made here [...]"

mchrostowski · 2019-11-06T21:23:33Z

Looking at the commit it's attributed in every way possible.

That happened after it was called out. Attribution has been important everywhere I've been in my life and I want to be sure the parties involved are aware of each other.

It's also bad etiquette to change a commit and not add your own name, because it's no longer the original work.

I fully agree and did not mean to suggest otherwise.
It actually looks like it was a lot more changes than I anticipated so I see why he nuked my commits and made one so I should not have made the comment.

I am greatly appreciative of @adamjonas for fixing the issue, referencing the original code, and the attribution as well as @fanquake for getting attention to the pull request. The code, though only 5 characters, took some time and I'm grateful to see it was not in vain. Seriously, when I had to rebase this thing I gave up (scary sneks) so it was exciting to see someone else grab it up.

adamjonas · 2019-11-06T22:01:25Z

@mchrostowski I agree that I was in the wrong when I opened the PR and mentioned the previous PR but didn't attribute you as the original author as fanquake pointed out (I was unaware of that tag). All I can say is that it wasn't meant as a slight and I'll be more intentional going forward.

…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

maflcko · 2019-11-11T20:32:14Z

@laanwj Does the "generate release notes contributors" read the Co-Authored-By: tag from commit bodies as well?

maflcko · 2019-11-11T20:32:29Z

Also, this should probably be backported to 0.19.1?

listsinceblock now checks that returned transactions are not conflicting with any transactions that are filtered out by the given blockhash Co-Authored-By: Michael Chrostowski <[email protected]> Github-Pull: bitcoin#17258 Rebased-From: 436ad43

fanquake · 2020-01-14T09:41:05Z

Being backported in 17792.

cd67b1d Use correct C++11 header for std::swap() (Hennadii Stepanov) b8101fb Fix comparison function signature (Hennadii Stepanov) eac4907 Don't allow implementers to think ScriptHash(Witness*()) results in nesting computation (Gregory Sanders) e2c45d8 IsUsedDestination shouldn't use key id as script id for ScriptHash (Gregory Sanders) a5489c9 IsUsedDestination should count any known single-key address (Gregory Sanders) 88729d8 Fix issue with conflicted mempool tx in listsinceblock (Adam Jonas) eafcea7 gui: Fix duplicate wallet showing up (João Barbosa) 7e66d04 Drop signal CClientUIInterface::LoadWallet (Russell Yanofsky) 179d55f zmq: Fix due to invalid argument and multiple notifiers (João Barbosa) Pull request description: Backports - #16963 - #17445 - #17258 - #17621 - #17924 - #17634 ACKs for top commit: laanwj: ACK cd67b1d, checked that I got more or less the same result (including conflict resolution) backporting these commits Tree-SHA512: 645786267cfb10a01a56f7cfd91ddead5f1475df5714595ae480237e04d40c5cfb7460b40532279cacd83e4b775a4ace68a258ec2184b8ad0e997a690a9245e5

laanwj · 2020-01-23T20:02:18Z

@laanwj Does the "generate release notes contributors" read the Co-Authored-By: tag from commit bodies as well?

No, it doesn't at the moment (probably should…).

…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

fanquake added Wallet RPC/REST/ZMQ labels Oct 25, 2019

adamjonas mentioned this pull request Oct 25, 2019

rpc listsinceblock hash is not correctly filtering double-spends #8752

Closed

adamjonas force-pushed the listsinceblock-filter-conflicts branch from 1d3f144 to 700a288 Compare October 25, 2019 19:58

fanquake requested review from TheBlueMatt and instagibbs October 25, 2019 20:33

instagibbs reviewed Oct 28, 2019

View reviewed changes

Fix issue with conflicted mempool tx in listsinceblock

436ad43

listsinceblock now checks that returned transactions are not conflicting with any transactions that are filtered out by the given blockhash Co-Authored-By: Michael Chrostowski <[email protected]>

adamjonas force-pushed the listsinceblock-filter-conflicts branch from 676991e to 436ad43 Compare October 28, 2019 14:27

fanquake requested a review from kallewoof October 29, 2019 12:50

kallewoof approved these changes Nov 2, 2019

View reviewed changes

jonatack reviewed Nov 4, 2019

View reviewed changes

meshcollider merged commit 436ad43 into bitcoin:master Nov 5, 2019

adamjonas deleted the listsinceblock-filter-conflicts branch November 5, 2019 21:24

MarkLTZ added a commit to litecoinz-core/litecoinz that referenced this pull request Nov 17, 2019

Fix issue with conflicted mempool tx in listsinceblock (bitcoin#17258)

92a27c3

fanquake added the Needs backport (0.19) label Jan 13, 2020

fanquake mentioned this pull request Jan 14, 2020

0.19: Backports #17792

Merged

fanquake removed the Needs backport (0.19) label Jan 14, 2020

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

maflcko mentioned this pull request May 8, 2020

listsinceblock shows conflicted transactions forever #11853

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Fix issue with conflicted mempool tx in listsinceblock #17258

Fix issue with conflicted mempool tx in listsinceblock #17258

Uh oh!

Conversation

adamjonas commented Oct 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Oct 25, 2019

Uh oh!

adamjonas commented Oct 25, 2019

Uh oh!

DrahtBot commented Oct 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs Oct 28, 2019

Choose a reason for hiding this comment

Uh oh!

adamjonas Oct 28, 2019

Choose a reason for hiding this comment

Uh oh!

instagibbs Oct 28, 2019

Choose a reason for hiding this comment

Uh oh!

adamjonas Oct 28, 2019

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Oct 28, 2019

Uh oh!

kallewoof commented Nov 2, 2019

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack Nov 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

meshcollider commented Nov 5, 2019

Uh oh!

mchrostowski commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Nov 6, 2019

Uh oh!

mchrostowski commented Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamjonas commented Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Nov 11, 2019

Uh oh!

maflcko commented Nov 11, 2019

Uh oh!

fanquake commented Jan 14, 2020

Uh oh!

laanwj commented Jan 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

adamjonas commented Oct 25, 2019 •

edited

Loading

DrahtBot commented Oct 25, 2019 •

edited

Loading

jonatack left a comment •

edited

Loading

jonatack Nov 4, 2019 •

edited

Loading

mchrostowski commented Nov 5, 2019 •

edited

Loading

kallewoof commented Nov 6, 2019 •

edited

Loading

laanwj commented Nov 6, 2019 •

edited

Loading

mchrostowski commented Nov 6, 2019 •

edited

Loading

adamjonas commented Nov 6, 2019 •

edited

Loading