Skip to content

Fix issue with conflicted mempool tx in listsinceblock #17258

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.

Sign up for GitHub

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

Merged
meshcollider merged 1 commit into from
Nov 5, 2019
Merged

Fix issue with conflicted mempool tx in listsinceblock #17258

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/wallet/rpcwallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

@jonatack jonatack Nov 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ListTransactions(*locked_chain, pwallet, tx, 0, true, transactions, filter, nullptr /* filter_label */);
}
}
Expand Down
49 changes: 49 additions & 0 deletions test/functional/wallet_listsinceblock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,15 @@
"""Test the listsincelast RPC."""

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

from decimal import Decimal
Copy link
Member

@jonatack jonatack Nov 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


class ListSinceBlockTest(BitcoinTestFramework):
def set_test_params(self):
Expand All @@ -33,6 +35,7 @@ def run_test(self):
self.test_reorg()
self.test_double_spend()
self.test_double_send()
self.double_spends_filtered()

def test_no_blockhash(self):
txid = self.nodes[2].sendtoaddress(self.nodes[0].getnewaddress(), 1)
Expand Down Expand Up @@ -291,5 +294,51 @@ def test_double_send(self):
if tx['txid'] == txid1:
assert_equal(tx['confirmations'], 2)

def double_spends_filtered(self):
'''
`listsinceblock` was returning conflicted transactions even if they
occurred before the specified cutoff blockhash
'''
spending_node = self.nodes[2]
dest_address = spending_node.getnewaddress()

tx_input = dict(
sequence=BIP125_SEQUENCE_NUMBER, **next(u for u in spending_node.listunspent()))
rawtx = spending_node.createrawtransaction(
[tx_input], {dest_address: tx_input["amount"] - Decimal("0.00051000"),
spending_node.getrawchangeaddress(): Decimal("0.00050000")})
signedtx = spending_node.signrawtransactionwithwallet(rawtx)
orig_tx_id = spending_node.sendrawtransaction(signedtx["hex"])
original_tx = spending_node.gettransaction(orig_tx_id)

double_tx = spending_node.bumpfee(orig_tx_id)

# check that both transactions exist
block_hash = spending_node.listsinceblock(
spending_node.getblockhash(spending_node.getblockcount()))
original_found = False
double_found = False
for tx in block_hash['transactions']:
if tx['txid'] == original_tx['txid']:
original_found = True
if tx['txid'] == double_tx['txid']:
double_found = True
assert_equal(original_found, True)
assert_equal(double_found, True)

lastblockhash = spending_node.generate(1)[0]

# check that neither transaction exists
block_hash = spending_node.listsinceblock(lastblockhash)
original_found = False
double_found = False
for tx in block_hash['transactions']:
if tx['txid'] == original_tx['txid']:
original_found = True
if tx['txid'] == double_tx['txid']:
double_found = True
assert_equal(original_found, False)
assert_equal(double_found, False)

if __name__ == '__main__':
ListSinceBlockTest().main()