Skip to content
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

Fix issue with conflicted mempool tx in listsinceblock #17258

Merged

Conversation

@adamjonas
Copy link
Contributor

adamjonas commented Oct 25, 2019

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

This comment has been minimized.

Copy link
Member

fanquake commented Oct 25, 2019

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

@adamjonas adamjonas force-pushed the adamjonas:listsinceblock-filter-conflicts branch from 1d3f144 to 700a288 Oct 25, 2019
@adamjonas

This comment has been minimized.

Copy link
Contributor Author

adamjonas commented Oct 25, 2019

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

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Oct 25, 2019

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.

@fanquake fanquake requested review from TheBlueMatt and instagibbs Oct 25, 2019
Copy link
Member

instagibbs left a comment

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?

from decimal import Decimal

# Sequence number that is BIP 125 opt-in and BIP 68-compliant
BIP125_SEQUENCE_NUMBER = 0xfffffffd

This comment has been minimized.

Copy link
@instagibbs

instagibbs Oct 28, 2019

Member

That should already be in test_framework.messages module

This comment has been minimized.

Copy link
@adamjonas

adamjonas Oct 28, 2019

Author Contributor

Right. Updated.

assert_equal(original_found, True)
assert_equal(double_found, True)

lastblockhash = spending_node.generate(6)[5]

This comment has been minimized.

Copy link
@instagibbs

This comment has been minimized.

Copy link
@adamjonas

adamjonas Oct 28, 2019

Author Contributor

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 <michael.chrostowski@gmail.com>
@adamjonas adamjonas force-pushed the adamjonas:listsinceblock-filter-conflicts branch from 676991e to 436ad43 Oct 28, 2019
@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Oct 28, 2019

utACK 436ad43

but someone with more history with the RPC call should review

@fanquake fanquake requested a review from kallewoof Oct 29, 2019
@kallewoof

This comment has been minimized.

Copy link
Member

kallewoof commented Nov 2, 2019

utACK 436ad43

Copy link
Member

jonatack left a comment

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.

@@ -1591,7 +1591,7 @@ static UniValue listsinceblock(const JSONRPCRequest& request)
for (const std::pair<const uint256, CWalletTx>& pairWtx : pwallet->mapWallet) {
CWalletTx tx = pairWtx.second;

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

This comment has been minimized.

Copy link
@jonatack

jonatack Nov 4, 2019

Member

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.

from test_framework.util import (
assert_array_result,
assert_equal,
assert_raises_rpc_error,
connect_nodes,
)

from decimal import Decimal

This comment has been minimized.

Copy link
@jonatack

jonatack Nov 4, 2019

Member

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

@meshcollider

This comment has been minimized.

Copy link
Member

meshcollider commented Nov 5, 2019

LGTM 436ad43

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
@meshcollider meshcollider merged commit 436ad43 into bitcoin:master Nov 5, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mchrostowski

This comment has been minimized.

Copy link
Contributor

mchrostowski commented Nov 5, 2019

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!

@adamjonas adamjonas deleted the adamjonas:listsinceblock-filter-conflicts branch Nov 5, 2019
@kallewoof

This comment has been minimized.

Copy link
Member

kallewoof commented Nov 6, 2019

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

This comment has been minimized.

Copy link
Member

laanwj commented Nov 6, 2019

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 <michael.chrostowski@gmail.com>

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

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Nov 6, 2019

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

This comment has been minimized.

Copy link
Contributor

mchrostowski commented Nov 6, 2019

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

This comment has been minimized.

Copy link
Contributor Author

adamjonas commented Nov 6, 2019

@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.

sidhujag added 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
@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 11, 2019

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 11, 2019

Also, this should probably be backported to 0.19.1?

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Nov 15, 2019
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 <michael.chrostowski@gmail.com>

Github-Pull: bitcoin#17258
Rebased-From: 436ad43
MarkLTZ added a commit to litecoinz-core/litecoinz that referenced this pull request Nov 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.