test: wallet rescan with reorged parent + IsFromMe child in mempool #29179

glozow · 2024-01-04T16:26:03Z

Originally motivated by #29019, which reverts back to having requestMempoolTransactions emit transactionAddedToMempool in mapTx default order instead of GetSortedDepthAndScore order.

It's important that these notifications happen in topological order, otherwise the wallet rescan may miss transactions that belong to it. Notably, checking whether a transaction IsFromMe requires knowing its inputs, which may be from a mempool parent.

When using mapTx order, a parent may come later than its child if it was added from a block disconnected in a reorg.

This PR adds a test for this case.

DrahtBot · 2024-01-04T16:26:06Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	furszy, achow101
Stale ACK	Eunovo, stickies-v

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28710 (Remove the legacy wallet and BDB dependency by achow101)

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.

achow101 · 2024-01-04T23:02:07Z

Huh, looks like this behavior has been broken since it was introduced in #15652

achow101 · 2024-01-04T23:16:40Z

ACK 1a52ca7619eeb1baafa5a32b364381126862b29d

stickies-v

Approach ACK, nice one finding this test case! 1a52ca7619eeb1baafa5a32b364381126862b29d looks good to me but not super familiar with this part of the code. Left some suggestions about maintainability and can be ignored.

I verified that the tests fail on #29019 as well as on 453b481~1, as is necessary.

test/functional/wallet_import_rescan.py

furszy

Pre-review note: have you considered that wallet_import_rescan.py is a legacy wallet only test?
If it's placed here due to a lack of alternatives, I think this could be moved to a separate wallet_mempool.py file, where we could continue adding more cases related to the wallet-mempool interaction (or.. we could upgrade this file to run on a descriptors wallet).

glozow · 2024-01-08T08:46:54Z

Pre-review note: have you considered that wallet_import_rescan.py is a legacy wallet only test?

Ah good point. ~~I added a commit to run this test with descriptors as I couldn't find a reason why we don't do this. Lmk if this seems sufficient / ok to do?~~ EDIT: oh descriptors can't use dumpprivkey, nvm

glozow · 2024-01-09T09:04:43Z

I'm working on adding a descriptors version of this test so we can have both.

glozow · 2024-01-09T10:39:16Z

I've added another commit which has pretty much the same test but using descriptor wallets i.e. a rescan through importdescriptors. Just like the other test, you can reproduce the bug by cherry-picking from #29019. You should get JSONRPCEXCEPTION: Invalid or non-wallet transaction id (-5) for the child tx.

stickies-v

Hooray for adding descriptor wallet support. Is there a reason both legacy and descriptor wallet tests aren't in the new wallet_rescan_unconfirmed test, though? That seems like a more logical grouping with less duplication? If there are technical objections, I think it would be good to add some extra documentation to both tests stating that the other wallet type is tested in file xxx.py, making it easier to clean up these tests e.g. when we drop legacy wallets or make other significant changes?

test/functional/wallet_rescan_unconfirmed.py

glozow · 2024-01-09T14:30:25Z

Is there a reason both legacy and descriptor wallet tests aren't in the new wallet_rescan_unconfirmed test, though? That seems like a more logical grouping with less duplication?

They use different RPCs. I think it's more readable for them to be separate (having most the test as if is_sqlite_compiled() elif is_bdb_compiled() is pretty annoying). I think it's much easier to delete legacy without touching the descriptor one if they're in different files.

Eunovo · 2024-01-10T22:20:22Z

Tested ACK d9df438

Ran the tests locally and got test_framework.authproxy.JSONRPCException: Invalid or non-wallet transaction id (-5) for both wallet_import_rescan.py and wallet_rescan_unconfirmed.py when insertion order is used in CTxMemPool::entryAll().

stickies-v

ACK d9df438c6e581dae0c818a4c2f5fe95627ae26bc

I think it's much easier to delete legacy without touching the descriptor one if they're in different files.

Thanks, you're right. There's less overlap than I thought there would be.

test/functional/wallet_rescan_unconfirmed.py

test/functional/wallet_import_rescan.py

test/functional/wallet_rescan_unconfirmed.py

…empool Test that wallet rescans process transactions topologically, even if a parent's entry into the mempool is later than that of its child. This behavior is important because IsFromMe requires the ability to look up a transaction's inputs.

stickies-v

re-ACK c39e829

glozow · 2024-01-12T10:35:28Z

CI failure is unrelated, #29234

furszy

Code reviewed, looking good. Left few comments, mostly testing time improvements. Nothing blocking.

As the scenario being covered does not require verifying p2p mempool relay, you could speed up the new wallet_rescan_unconfirmed.py test by ~2.5x (tested locally, ~3 secs versus ~8 secs) by using a single node instead of creating three of them. See commit f827351 (feel free to pull it).

test/functional/wallet_rescan_unconfirmed.py

stickies-v

re-ACK 9db0d42509e0e223b45238fd8087fc98ef32c091

…in mempool Test that wallet rescans process transactions topologically, even if a parent's entry into the mempool is later than that of its child. This behavior is important because IsFromMe requires the ability to look up a transaction's inputs. Co-authored-by: furszy <[email protected]>

glozow · 2024-01-12T14:53:09Z

See commit f827351 (feel free to pull it).

Sorry I hadn't seen this. Thanks, taken with a couple of edits.

furszy

Code review ACK df30247, nits can be disregarded.

furszy · 2024-01-12T19:03:57Z

test/functional/wallet_rescan_unconfirmed.py

+        # inputs of the transaction to detect it, so the parent must be processed before the child.
+        w0_utxos = w0.listunspent()
+
+        self.log.info("Create a child tx and wait for it to propagate to all mempools")


nit:
leftover

Suggested change

self.log.info("Create a child tx and wait for it to propagate to all mempools")

self.log.info("Create a child tx and wait until all wallets are notified")

furszy · 2024-01-12T19:04:49Z

test/functional/wallet_rescan_unconfirmed.py

+        node.invalidateblock(block_to_reorg)
+        assert tx_parent_to_reorg["txid"] in node.getrawmempool()
+
+        self.log.info("Import descriptor wallet on another node")


nit:
leftover; should be "import descriptor on another wallet"

achow101 · 2024-01-16T17:38:39Z

ACK df30247

stickies-v

Post-merge re-ACK df30247, I was still investigating the non-deterministic behaviour in wallet_import_rescan but I've now found the issue and it doesn't seem problematic enough to revert or immediately fix.

When rebasing these tests on top of e.g. #29019 as well as on 453b4813ebc74859864803e9972b58e4be76a4d6~1, the mempool variant tests should fail for every variant where expect_rescan == True because in each case, only the parent is re-imported in the mempool (after re-org). However, when running the test multiple times one can observe this is not the case, and it sometimes takes 2 or even more rescan variants before the test fails.

log outputs

Expected failure logs:

...
2024-01-18T13:20:15.028000Z TestFramework (INFO): Test that the mempool is rescanned as well if the rescan parameter is set to true
2024-01-18T13:20:30.271000Z TestFramework (INFO): Run import for mempool variant Variant(call=<Call.single: 1>, data=<Data.address: 1>, address_type=<AddressType.bech32: 'bech32'>, rescan=<Rescan.no: 1>, prune=False)
2024-01-18T13:20:30.317000Z TestFramework (INFO): Run import for mempool variant Variant(call=<Call.single: 1>, data=<Data.address: 1>, address_type=<AddressType.bech32: 'bech32'>, rescan=<Rescan.yes: 2>, prune=False)
2024-01-18T13:20:30.388000Z TestFramework (ERROR): JSONRPC error
...

Actual failure logs, sometimes:

...
2024-01-18T13:25:50.799000Z TestFramework (INFO): Test that the mempool is rescanned as well if the rescan parameter is set to true
2024-01-18T13:26:06.108000Z TestFramework (INFO): Run import for mempool variant Variant(call=<Call.single: 1>, data=<Data.address: 1>, address_type=<AddressType.bech32: 'bech32'>, rescan=<Rescan.no: 1>, prune=False)
2024-01-18T13:26:06.164000Z TestFramework (INFO): Run import for mempool variant Variant(call=<Call.single: 1>, data=<Data.address: 1>, address_type=<AddressType.bech32: 'bech32'>, rescan=<Rescan.yes: 2>, prune=False)
2024-01-18T13:26:06.235000Z TestFramework (INFO): Run import for mempool variant Variant(call=<Call.single: 1>, data=<Data.address: 1>, address_type=<AddressType.p2sh_segwit: 'p2sh-segwit'>, rescan=<Rescan.no: 1>, prune=False)
2024-01-18T13:26:06.268000Z TestFramework (INFO): Run import for mempool variant Variant(call=<Call.single: 1>, data=<Data.address: 1>, address_type=<AddressType.p2sh_segwit: 'p2sh-segwit'>, rescan=<Rescan.yes: 2>, prune=False)
2024-01-18T13:26:06.341000Z TestFramework (INFO): Run import for mempool variant Variant(call=<Call.single: 1>, data=<Data.address: 1>, address_type=<AddressType.legacy: 'legacy'>, rescan=<Rescan.no: 1>, prune=False)
2024-01-18T13:26:06.374000Z TestFramework (INFO): Run import for mempool variant Variant(call=<Call.single: 1>, data=<Data.address: 1>, address_type=<AddressType.legacy: 'legacy'>, rescan=<Rescan.yes: 2>, prune=False)
2024-01-18T13:26:06.449000Z TestFramework (INFO): Run import for mempool variant Variant(call=<Call.single: 1>, data=<Data.pub: 2>, address_type=<AddressType.bech32: 'bech32'>, rescan=<Rescan.no: 1>, prune=False)
2024-01-18T13:26:06.494000Z TestFramework (INFO): Run import for mempool variant Variant(call=<Call.single: 1>, data=<Data.pub: 2>, address_type=<AddressType.bech32: 'bech32'>, rescan=<Rescan.yes: 2>, prune=False)
2024-01-18T13:26:06.578000Z TestFramework (ERROR): JSONRPC error
...

This happens because a chain of transactions can be formed, where variant_a.child_tx == variant_b.initial_txid, causing variant_a's child tx to be re-imported into the mempool when variant b's initial tx is re-orged.

I don't think this needs to be fixed or reverted, because if such a chain is formed, the last variant in the chain is still guaranteed to fail. The descriptor test is unaffected because we create only a single parent/child combo.

stickies-v · 2024-01-19T14:26:49Z

Reviewers might be interested in #29283, fixing a rare intermittency issue

…empool Test that wallet rescans process transactions topologically, even if a parent's entry into the mempool is later than that of its child. This behavior is important because IsFromMe requires the ability to look up a transaction's inputs. Github-Pull: bitcoin#29179 Rebased-From: c3d02be

…in mempool Test that wallet rescans process transactions topologically, even if a parent's entry into the mempool is later than that of its child. This behavior is important because IsFromMe requires the ability to look up a transaction's inputs. Co-authored-by: furszy <[email protected]> Github-Pull: bitcoin#29179 Rebased-From: df30247

11f3a7e Use hardened runtime on macOS release builds. (Mark Friedenbach) ac1b9a5 [test] import descriptor wallet with reorged parent + IsFromMe child in mempool (glozow) ecb8ebc [test] rescan legacy wallet with reorged parent + IsFromMe child in mempool (Gloria Zhao) 438ac29 snapshots: don't core dump when running -checkblockindex after `loadtxoutset` (Mark Friedenbach) 7ec3455 [log] mempool loading (glozow) fe0f8fe net: create I2P sessions with both ECIES-X25519 and ElGamal encryption (Jon Atack) fc62271 [refactor] Add helper for iterating through mempool entries (stickies-v) Pull request description: Backports for 26.x. Includes: - 453b481 from #28391 - #29179 - #29200 - #29227 - #28791 - #29127 ACKs for top commit: stickies-v: ACK 11f3a7e Tree-SHA512: 20ef871ec768f2328056d83f958e595b36ae9b8baa8a6e8b0e1f02c3df4b16965a8e05dcb4323afbcc9ecc4bdde10931232512022c39ee7e12baf9795bf25bf1

glozow added the Tests label Jan 4, 2024

glozow mentioned this pull request Jan 4, 2024

mempool: Don't sort in entryAll #29019

Closed

stickies-v reviewed Jan 5, 2024

View reviewed changes

glozow force-pushed the 2024-01-test-reorg-rescan branch from 1a52ca7 to aa7eda3 Compare January 5, 2024 18:14

furszy reviewed Jan 5, 2024

View reviewed changes

DrahtBot mentioned this pull request Jan 6, 2024

Remove the legacy wallet and BDB dependency #28710

Merged

glozow force-pushed the 2024-01-test-reorg-rescan branch from b4eafa3 to aa7eda3 Compare January 8, 2024 09:18

DrahtBot added CI failed and removed CI failed labels Jan 8, 2024

glozow force-pushed the 2024-01-test-reorg-rescan branch from aa7eda3 to 86f5cc8 Compare January 9, 2024 10:36

glozow force-pushed the 2024-01-test-reorg-rescan branch from 86f5cc8 to 1b18ac4 Compare January 9, 2024 10:55

DrahtBot added the CI failed label Jan 9, 2024

glozow force-pushed the 2024-01-test-reorg-rescan branch from 1b18ac4 to fa2b8d0 Compare January 9, 2024 11:07

stickies-v reviewed Jan 9, 2024

View reviewed changes

test/functional/wallet_rescan_unconfirmed.py Outdated Show resolved Hide resolved

test/functional/wallet_rescan_unconfirmed.py Outdated Show resolved Hide resolved

test/functional/wallet_rescan_unconfirmed.py Outdated Show resolved Hide resolved

DrahtBot removed the CI failed label Jan 9, 2024

glozow force-pushed the 2024-01-test-reorg-rescan branch from fa2b8d0 to d9df438 Compare January 9, 2024 15:48

glozow requested review from achow101, furszy and stickies-v January 10, 2024 10:46

stickies-v approved these changes Jan 11, 2024

View reviewed changes

test/functional/wallet_rescan_unconfirmed.py Outdated Show resolved Hide resolved

test/functional/wallet_import_rescan.py Outdated Show resolved Hide resolved

test/functional/wallet_rescan_unconfirmed.py Outdated Show resolved Hide resolved

glozow force-pushed the 2024-01-test-reorg-rescan branch from d9df438 to c39e829 Compare January 12, 2024 09:55

stickies-v approved these changes Jan 12, 2024

View reviewed changes

furszy reviewed Jan 12, 2024

View reviewed changes

glozow force-pushed the 2024-01-test-reorg-rescan branch from c39e829 to 9db0d42 Compare January 12, 2024 14:23

stickies-v approved these changes Jan 12, 2024

View reviewed changes

glozow force-pushed the 2024-01-test-reorg-rescan branch from 9db0d42 to 4d09fd0 Compare January 12, 2024 14:46

DrahtBot added the CI failed label Jan 12, 2024

glozow force-pushed the 2024-01-test-reorg-rescan branch from 4d09fd0 to df30247 Compare January 12, 2024 14:52

DrahtBot removed the CI failed label Jan 12, 2024

furszy reviewed Jan 12, 2024

View reviewed changes

DrahtBot requested a review from stickies-v January 12, 2024 19:07

DrahtBot removed the request for review from achow101 January 16, 2024 17:38

achow101 merged commit 27d935f into bitcoin:master Jan 16, 2024

glozow mentioned this pull request Jan 18, 2024

[26.x] more backports #29209

Merged

stickies-v reviewed Jan 18, 2024

View reviewed changes

bitcoin locked and limited conversation to collaborators Feb 7, 2025

	self.log.info("Create a child tx and wait for it to propagate to all mempools")
	self.log.info("Create a child tx and wait until all wallets are notified")

test: wallet rescan with reorged parent + IsFromMe child in mempool #29179

test: wallet rescan with reorged parent + IsFromMe child in mempool #29179

Uh oh!

Conversation

glozow commented Jan 4, 2024

Uh oh!

DrahtBot commented Jan 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

achow101 commented Jan 4, 2024

Uh oh!

achow101 commented Jan 4, 2024

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

glozow commented Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glozow commented Jan 9, 2024

Uh oh!

glozow commented Jan 9, 2024

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glozow commented Jan 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Eunovo commented Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

glozow commented Jan 12, 2024

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

glozow commented Jan 12, 2024

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

furszy Jan 12, 2024

Choose a reason for hiding this comment

Uh oh!

furszy Jan 12, 2024

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jan 16, 2024

DrahtBot commented Jan 4, 2024 •

edited

Loading

glozow commented Jan 8, 2024 •

edited

Loading

glozow commented Jan 9, 2024 •

edited

Loading

Eunovo commented Jan 10, 2024 •

edited

Loading

stickies-v left a comment •

edited

Loading