wallet: coverage for receiving txes with same id but different witness data #25909

furszy · 2022-08-23T14:16:13Z

Based on #11240 context, adding test coverage for the behavior introduced in #11225 and to the current wallet limitations.

This is the first step towards adding the ability to store multiple transactions with same tx id but different witness data in the wallet. Verifying and testing the current behavior before introducing the new features.

The following cases are covered:

Two p2wpkh spending transactions with the same hash are received:
The first one with segwit data stripped, and the second one with segwit data.

Result -> the wallet will update the stored tx, saving the witness data.
Two p2wsh multisig spending txes with the same hash but a different witness are received:
The first is added to the wallet via the mempool acceptance flow.
while the second one, is added to the wallet via the block connection flow.

Result -> the wallet will NOT update the stored transaction. The first received transaction
will take precedence over any following-up transaction. Detached to the fact that the
original transaction didn't get into a block and the second one did.

Extra Note:
Did it on an unit test merely because wanted to review other parts of the sources while was doing it. Could migrate it into a functional test if reviewers wants it as well.

DrahtBot · 2022-08-23T15:29:19Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	satsie
Approach ACK	w0xlt
Stale ACK	aureleoules

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

Conflicts

No conflicts as of last run.

w0xlt

Approach ACK

src/wallet/scriptpubkeyman.h

src/wallet/test/wallet_transaction_tests.cpp

satsie · 2022-09-01T02:05:45Z

I've reviewed the diff (398e247), checked out the code and run the tests (using ./src/test/test_bitcoin --log_level=all --run_test=wallet_transaction_tests). As an extremely new (aspiring) contributor with limited knowledge, this all looks fine to me. However I don't feel comfortable enough in my own abilities to give it a real ACK.

I added extra assertions locally for my own learning. Didn't want to clutter the diff with individual code comments, so I will share them here. I don't know if they are even useful or just redundant. For me, the main thing is it is not immediately obvious that the existing assertions cover the expected presence/absence of witness data.

The assertions on the transaction IDs (like txid in the first test with the two p2wpkh transactions) make sense to anyone who understands that when there is no witness data, the witness hash is the same as the tx hash (per transaction.cpp.ComputeWitnessHash()). I'm not sure how obvious this is others reading the code, or if there are inefficiencies related to explicitly checking the witness data. It could be a non issue for those that spend more time here than me.

Apologies in advance if there are any C++ gotchas with pointers and references that I am not aware of :)

	// From the first transaction in the first test case:
        BOOST_CHECK(wtx_no_witness->GetWitnessHash() == txid);
        BOOST_CHECK(wtx_no_witness->tx->HasWitness() == 0); // additional assertion to verify that the transaction in the wallet has no witness data

	// From the second transaction in the first test case:
        BOOST_CHECK(wtx_with_witness->GetWitnessHash() != txid);
        BOOST_CHECK(wtx_with_witness->tx->HasWitness() == 1); // additional assertion to verify that the transaction in the wallet now has witness data

Aside from that I had a few questions to help me understand the PR better:

wallet_transaction_tests.CreateMultisigScriptAndImportDescriptor: Is there any reason why this test code creates a 3-of-5 multisig instead of a 2-of-3?
Was the removal of src/test/util/wallet.h and src/test/util.wallet.cpp part of routine cleanup? Because there is a src/wallet/test folder with way more in it and it doesn't make sense to have wallet unit tests in two places?

furszy · 2022-09-02T14:21:45Z

The assertions on the transaction IDs (like txid in the first test with the two p2wpkh transactions) make sense to anyone who understands that when there is no witness data, the witness hash is the same as the tx hash (per transaction.cpp.ComputeWitnessHash()). I'm not sure how obvious this is others reading the code, or if there are inefficiencies related to explicitly checking the witness data. It could be a non issue for those that spend more time here than me.

Yep, non-issue. It comes from the segwit definitions (BIP141).
The transaction id is the hash of the transaction data serialized. The witness transaction id is the hash of the transaction data serialized + a marker, flag and witness data.

if the witness data doesn't exist (or isn't there), then you are serializing only the regular transaction fields, thus why the tx id is equal to the witness id.

wallet_transaction_tests.CreateMultisigScriptAndImportDescriptor: Is there any reason why this test code creates a 3-of-5 multisig instead of a 2-of-3?

Not really. For the test purposes, it could had been a 1-of-2 as well. I just liked more to present a scenario where the user looses data that is hard to re-do if keys are spread across different devices/places.

Was the removal of src/test/util/wallet.h and src/test/util.wallet.cpp part of routine cleanup? Because there is a src/wallet/test folder with way more in it and it doesn't make sense to have wallet unit tests in two places?

Both files were just containing utility functions to interact with the wallet. None of them are unit tests.
They were sharing the same purpose and the wallet code should be placed inside the wallet directory.

Plus, as wallet/test/util.h and wallet/test/util.cpp were not part of the util library, we weren't able to use the functions on the bench and unit test modules (them are on different units). Which would had forced me to duplicate the DuplicateMockDatabase function that was inside of one of benchmarks to be able to use it in the new unit test case.

furszy · 2022-10-27T14:12:16Z

rebased, conflicts solved.

aureleoules

ACK 326d030

Great refactoring and I verified that the tests correctly test the behavior intended.

src/wallet/test/wallet_transaction_tests.cpp

furszy · 2022-11-03T15:21:39Z

Updated per @aureleoules feedback, thanks!

Tiny diff. Only moved from using the test coinbase key to use the custom one, no functional changes.

aureleoules

reACK 312f367

…ness data The following cases are covered: 1) Two p2wpkh transactions with the same hash are received: The first one with segwit data stripped, and the second one with segwit data. Result -> the wallet will update the stored tx, saving the witness data. 2) Two p2wsh multisig spending txes with the same hash but a different witness are received: The first is added to the wallet via the mempool acceptance flow. while the second one, is added to the wallet via the block connection flow. Result -> the wallet will NOT update the stored transaction, the first received transaction will take precedence over any following-up transaction. Don't care if the original transaction didn't get into a block and the second one did.

achow101

Result -> the wallet will NOT update the stored transaction. The first received transaction
will take precedence over any following-up transaction. Detached to the fact that the
original transaction didn't get into a block and the second one did.

While it's good to have tests for the behavior that occurs, having tests for incorrect behavior is probably not something we want to be doing as it can make the bug last a lot longer. If the intention is to have a followup soon that fixes the bug, then I would prefer that these test changes just get rolled into that rather than be their own PR. Otherwise the test needs a big warning that it is testing actual incorrect behavior, and not intended behavior.

achow101 · 2023-01-24T19:22:05Z

src/wallet/test/wallet_transaction_tests.cpp

+    std::string pks_str; // descriptor data
+    for (int i=0; i < 5; i++) {
+        CTxDestination multi_dest = *Assert(spkm->GetNewDestination(OutputType::BECH32));
+        auto provider = Assert(spkm->GetSolvingProvider(GetScriptForDestination(multi_dest), /*include_private=*/ true));


I would strongly prefer that we don't have to add new methods to DescSPKM that can export individual keys. This test could just as easily work with generating new random CKeys for the private keys.

maflcko · 2023-09-20T17:38:30Z

Are you still working on this?

furszy · 2023-09-21T10:42:43Z

Are you still working on this?

not really. Closing for now.

furszy mentioned this pull request Aug 23, 2022

Wallet should be able to store multiple transactions with same txid #11240

Open

furszy changed the title ~~wallet: test coverage for receiving txes with same id but different witness data~~ wallet: coverage for receiving txes with same id but different witness data Aug 23, 2022

DrahtBot added the Wallet label Aug 23, 2022

This was referenced Aug 23, 2022

Add pool based memory resource #25325

Merged

wallet: Load database records in a particular order #24914

Merged

w0xlt reviewed Aug 23, 2022

View reviewed changes

src/wallet/scriptpubkeyman.h Outdated Show resolved Hide resolved

furszy force-pushed the 2022_wallet_test_witness_data_storage branch 3 times, most recently from cd2ce2e to 398e247 Compare August 25, 2022 02:22

DrahtBot mentioned this pull request Aug 26, 2022

wallet: Faster transaction creation by removing pre-set-inputs fetching responsibility from Coin Selection #25685

Merged

satsie reviewed Sep 1, 2022

View reviewed changes

src/wallet/test/wallet_transaction_tests.cpp Show resolved Hide resolved

DrahtBot mentioned this pull request Sep 1, 2022

[WIP] wallet: standardize change output detection process #25979

Closed

3 tasks

DrahtBot mentioned this pull request Sep 22, 2022

Reduce wasted pseudorandom bytes in ChaCha20 + various improvements #26153

Merged

DrahtBot added the Needs rebase label Oct 19, 2022

furszy force-pushed the 2022_wallet_test_witness_data_storage branch from 398e247 to 9530539 Compare October 27, 2022 14:12

DrahtBot added Needs rebase and removed Needs rebase labels Oct 27, 2022

furszy force-pushed the 2022_wallet_test_witness_data_storage branch 2 times, most recently from 651ff6a to 326d030 Compare November 1, 2022 18:20

DrahtBot removed the Needs rebase label Nov 1, 2022

aureleoules approved these changes Nov 3, 2022

View reviewed changes

src/wallet/test/wallet_transaction_tests.cpp Show resolved Hide resolved

furszy force-pushed the 2022_wallet_test_witness_data_storage branch from 326d030 to 312f367 Compare November 3, 2022 15:20

aureleoules approved these changes Nov 3, 2022

View reviewed changes

DrahtBot mentioned this pull request Nov 20, 2022

wallet: bugfix, invalid crypted key "checksum_valid" set #26532

Merged

DrahtBot added the Needs rebase label Nov 30, 2022

furszy force-pushed the 2022_wallet_test_witness_data_storage branch from 312f367 to d811ec8 Compare December 3, 2022 23:08

DrahtBot removed the Needs rebase label Dec 3, 2022

achow101 reviewed Jan 24, 2023

View reviewed changes

DrahtBot mentioned this pull request Feb 11, 2023

Wallet: estimate the size of signed inputs using descriptors #26567

Merged

furszy marked this pull request as draft July 19, 2023 13:47

furszy closed this Sep 21, 2023

bitcoin locked and limited conversation to collaborators Sep 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

wallet: coverage for receiving txes with same id but different witness data #25909

wallet: coverage for receiving txes with same id but different witness data #25909

Uh oh!

furszy commented Aug 23, 2022 •

edited

Loading

Uh oh!

DrahtBot commented Aug 23, 2022 •

edited

Loading

Uh oh!

w0xlt left a comment

Uh oh!

Uh oh!

Uh oh!

satsie commented Sep 1, 2022

Uh oh!

furszy commented Sep 2, 2022 •

edited

Loading

Uh oh!

furszy commented Oct 27, 2022

Uh oh!

aureleoules left a comment

Uh oh!

Uh oh!

furszy commented Nov 3, 2022

Uh oh!

aureleoules left a comment

Uh oh!

achow101 left a comment •

edited

Loading

Uh oh!

achow101 Jan 24, 2023

Uh oh!

maflcko commented Sep 20, 2023

Uh oh!

furszy commented Sep 21, 2023

Uh oh!

Uh oh!

wallet: coverage for receiving txes with same id but different witness data #25909

wallet: coverage for receiving txes with same id but different witness data #25909

Uh oh!

Conversation

furszy commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

satsie commented Sep 1, 2022

Uh oh!

furszy commented Sep 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

furszy commented Oct 27, 2022

Uh oh!

aureleoules left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

furszy commented Nov 3, 2022

Uh oh!

aureleoules left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 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!

achow101 Jan 24, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko commented Sep 20, 2023

Uh oh!

furszy commented Sep 21, 2023

Uh oh!

Uh oh!

furszy commented Aug 23, 2022 •

edited

Loading

DrahtBot commented Aug 23, 2022 •

edited

Loading

furszy commented Sep 2, 2022 •

edited

Loading

achow101 left a comment •

edited

Loading