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.
Co-Authored-By: Author Name <email@address.com> to the commit body.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
10@@ -11,6 +11,10 @@
11 assert_raises_rpc_error,
12 connect_nodes,
13 )
14+from decimal import Decimal
15+
16+# Sequence number that is BIP 125 opt-in and BIP 68-compliant
17+BIP125_SEQUENCE_NUMBER = 0xfffffffd
test_framework.messages module
326+ if tx['txid'] == double_tx['txid']:
327+ double_found = True
328+ assert_equal(original_found, True)
329+ assert_equal(double_found, True)
330+
331+ lastblockhash = spending_node.generate(6)[5]
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>
utACK https://github.com/bitcoin/bitcoin/pull/17258/commits/436ad436434b94982bcb7dc1d13a21949263ef73
but someone with more history with the RPC call should review
1590@@ -1591,7 +1591,7 @@ static UniValue listsinceblock(const JSONRPCRequest& request)
1591 for (const std::pair<const uint256, CWalletTx>& pairWtx : pwallet->mapWallet) {
1592 CWalletTx tx = pairWtx.second;
1593
1594- if (depth == -1 || tx.GetDepthInMainChain(*locked_chain) < depth) {
1595+ if (depth == -1 || abs(tx.GetDepthInMainChain(*locked_chain)) < depth) {
11 assert_equal,
12 assert_raises_rpc_error,
13 connect_nodes,
14 )
15
16+from decimal import Decimal
I’m not qualifed to give an ACK here but 436ad436434b94982bcb7dc1d13a21949263ef73 appears reasonable. Built/ran tests/verified that this test fails without the change in rpcwallet.cpp:
0File "./wallet_listsinceblock.py", line 340, in double_spends_filtered
1 assert_equal(original_found, False)
Feel free to ignore the comments below.
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!
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.
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.
0Fix issue with conflicted mempool tx in listsinceblock
1[@adamjonas](/bitcoin-bitcoin/contributor/adamjonas/)
2[@mchrostowski](/bitcoin-bitcoin/contributor/mchrostowski/)
3adamjonas and mchrostowski committed 16 days ago
4
5listsinceblock now checks that returned transactions are not
6conflicting with any transactions that are filtered out by
7the given blockhash
8
9Co-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.
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 […]”
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.
Co-Authored-By: tag from commit bodies as well?
