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

furszy commented at 2:16 PM on August 23, 2022: member

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.

furszy renamed this:
~~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
on Aug 23, 2022

DrahtBot commented at 3:29 PM on August 23, 2022: contributor

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.

DrahtBot added the label Wallet on Aug 23, 2022

in src/wallet/scriptpubkeyman.h:612 in 9024059e9e outdated

 608 | @@ -609,6 +609,7 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
 609 |      bool CanGetAddresses(bool internal = false) const override;
 610 |  
 611 |      std::unique_ptr<SigningProvider> GetSolvingProvider(const CScript& script) const override;
 612 | +    std::unique_ptr<SigningProvider> GetSolvingProvider(const CScript& script, bool include_private) const { return GetSigningProvider(script, true); }

w0xlt commented at 8:47 PM on August 23, 2022:

include_private parameter is not being used.

    std::unique_ptr<SigningProvider> GetSolvingProvider(const CScript& script, bool include_private) const { return GetSigningProvider(script, include_private); }

furszy commented at 1:57 PM on August 24, 2022:

ups, thanks. Pushed.

w0xlt commented at 8:47 PM on August 23, 2022: contributor

Approach ACK

furszy force-pushed on Aug 24, 2022

furszy force-pushed on Aug 25, 2022

in src/wallet/test/wallet_transaction_tests.cpp:186 in 398e247665 outdated

 182 | +
 183 | +        // Important: current wallet behavior will NOT update the input witness data.
 184 | +        // The wallet will only see the witness data of the first seen tx.
 185 | +        BOOST_CHECK(wtx->GetHash() == signed_tx_2.GetHash());
 186 | +        BOOST_CHECK(wtx->GetWitnessHash() != MakeTransactionRef(signed_tx_2)->GetWitnessHash());
 187 | +    }

satsie commented at 2:03 AM on September 1, 2022:

In the other test case with the two p2wpkh transactions, there is a step at the end to reload the wallet from disk. Is there any value in doing the same here?

        // Reload the wallet as it would be reloaded from disk and check that the witness data from the first transaction is still there.
        // (flush the previous wallet first)
        wallet->Flush();
        DatabaseOptions options;
        std::unique_ptr<CWallet> wallet_reloaded = std::make_unique<CWallet>(m_node.chain.get(), "", m_args,
                                                                             DuplicateMockDatabase(wallet->GetDatabase(),options));
        BOOST_ASSERT(wallet_reloaded->LoadWallet() == DBErrors::LOAD_OK);
        const CWalletTx* reloaded_wtx_with_original_witness = WITH_LOCK(wallet_reloaded->cs_wallet, return wallet_reloaded->GetWalletTx(wtx->GetHash()));
        BOOST_CHECK_EQUAL(reloaded_wtx_with_original_witness->GetWitnessHash(), MakeTransactionRef(signed_tx_1)->GetWitnessHash());

furszy commented at 1:12 PM on September 2, 2022:

Nop, there is no value doing that here. The test here validates the current limitation of not updating the existent transaction. As there is no wtx update, there is no write to disk, there by there is no reason to test a reload.

And even if the wallet limitation wouldn't exist, the first reload test case is enough to verify the record durability. Sources flow wise, replace a tx with no witness is exactly the same as replacing a tx with witness.

satsie commented at 2:05 AM on September 1, 2022: contributor

I've reviewed the diff (398e2476657a761d47942936c8832ec7229291e3), 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 commented at 2:21 PM on September 2, 2022: member

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.

DrahtBot added the label Needs rebase on Oct 19, 2022

furszy force-pushed on Oct 27, 2022

furszy commented at 2:12 PM on October 27, 2022: member

rebased, conflicts solved.

DrahtBot removed the label Needs rebase on Oct 27, 2022

DrahtBot added the label Needs rebase on Oct 27, 2022

furszy force-pushed on Nov 1, 2022

DrahtBot removed the label Needs rebase on Nov 1, 2022

in src/wallet/test/wallet_transaction_tests.cpp:27 in 326d03085a outdated

  23 | +    const CBlock& block = context.CreateAndProcessBlock({tx_to_mine}, coinbase_script);
  24 | +    wallet.blockConnected(kernel::MakeBlockInfo(WITH_LOCK(cs_main, return context.m_node.chainman->ActiveChain().Tip()), &block));
  25 | +    return Assert(WITH_LOCK(wallet.cs_wallet, return wallet.GetWalletTx(tx_to_mine.GetHash())));
  26 | +}
  27 | +
  28 | +const CWalletTx* SendCoinsAndGenBlock(TestChain100Setup& context, const CScript& scriptpubkey, CAmount amount, CWallet& from_wallet, const CScript& coinbase_script)

aureleoules commented at 2:28 PM on November 2, 2022:

coinbase_script is unused

furszy commented at 3:20 PM on November 3, 2022:

pushed, thanks

aureleoules approved

aureleoules commented at 9:56 AM on November 3, 2022: member

ACK 326d03085a2e41d021d42f8ce324df235c907b19

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

furszy force-pushed on Nov 3, 2022

furszy commented at 3:21 PM on November 3, 2022: member

Updated per @aureleoules feedback, thanks!

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

aureleoules approved

aureleoules commented at 3:23 PM on November 3, 2022: member

reACK 312f3673618c83ddfcbc5e2c23722029bed47eb4

DrahtBot added the label Needs rebase on Nov 30, 2022

wallet: test coverage for receive txes with same id but different witness 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.

d811ec8fb6

furszy force-pushed on Dec 3, 2022

DrahtBot removed the label Needs rebase on Dec 3, 2022

in src/wallet/test/wallet_transaction_tests.cpp:41 in d811ec8fb6

  37 | +    auto spkm = static_cast<DescriptorScriptPubKeyMan*>(wallet.GetScriptPubKeyMan(OutputType::BECH32, false));
  38 | +    std::vector<CPubKey> pks;
  39 | +    std::string pks_str; // descriptor data
  40 | +    for (int i=0; i < 5; i++) {
  41 | +        CTxDestination multi_dest = *Assert(spkm->GetNewDestination(OutputType::BECH32));
  42 | +        auto provider = Assert(spkm->GetSolvingProvider(GetScriptForDestination(multi_dest), /*include_private=*/ true));

achow101 commented at 7:22 PM on January 24, 2023:

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.

achow101 commented at 7:24 PM on January 24, 2023: member

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.

furszy marked this as a draft on Jul 19, 2023

maflcko commented at 5:38 PM on September 20, 2023: member

Are you still working on this?

furszy commented at 10:42 AM on September 21, 2023: member

Are you still working on this?

not really. Closing for now.

furszy closed this on Sep 21, 2023

bitcoin locked this on Sep 20, 2024