wallet: update stored witness in AddToWallet #11225

1. 
   sdaftuar commented at 12:59 PM on September 3, 2017: member

   Replace witness-stripped wallet transactions with full transactions; this can happen when upgrading from a pre-segwit wallet to a segwit- aware wallet.

2. wallet: update stored witness in AddToWallet

   Replace witness-stripped wallet transactions with full transactions;
   this can happen when upgrading from a pre-segwit wallet to a segwit-
   aware wallet.

   d01a9682b1
3. fanquake added the label Wallet on Sep 3, 2017
4. 
   jonasschnelli commented at 5:37 PM on September 3, 2017: contributor

   utACK 617c459c6c56911e70855dbe139ab095f435e73e. Makes sense. Ideally we should add a test for this.

5. 
   sipa commented at 9:21 PM on September 3, 2017: member

   utACK d01a9682b126a5f83c7311e652e6e62f2c2e1d20

   This should be backported to 0.15(.1), I think.

6. in src/wallet/wallet.cpp:919 in d01a9682b1

    913 | @@ -914,6 +914,15 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
    914 |              wtx.fFromMe = wtxIn.fFromMe;
    915 |              fUpdated = true;
    916 |          }
    917 | +        // If we have a witness-stripped version of this transaction, and we
    918 | +        // see a new version with a witness, then we must be upgrading a pre-segwit
    919 | +        // wallet.  Store the new version of the transaction with the witness,
   

   ---

   ---

   

   promag commented at 9:55 AM on September 4, 2017:

   Remove extra space.

7. in src/wallet/wallet.cpp:922 in d01a9682b1

    913 | @@ -914,6 +914,15 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
    914 |              wtx.fFromMe = wtxIn.fFromMe;
    915 |              fUpdated = true;
    916 |          }
    917 | +        // If we have a witness-stripped version of this transaction, and we
    918 | +        // see a new version with a witness, then we must be upgrading a pre-segwit
    919 | +        // wallet.  Store the new version of the transaction with the witness,
    920 | +        // as the stripped-version must be invalid.
    921 | +        // TODO: Store all versions of the transaction, instead of just one.
    922 | +        if (wtxIn.tx->HasWitness() && !wtx.tx->HasWitness()) {
   

   ---

   ---

   

   promag commented at 10:05 AM on September 4, 2017:

   First check stored transaction:

   if (!wtx.tx->HasWitness() && wtxIn.tx->HasWitness()) {
   

8. 
   promag commented at 10:08 AM on September 4, 2017: member

   BTW, remove TODO comment and create an issue since it requires a significant change/discussion?

9. 
   sdaftuar commented at 4:14 PM on September 5, 2017: member

   @promag I opened #11240 to expand on the issue mentioned in the TODO.

10. MarcoFalke added this to the milestone 0.15.1 on Sep 5, 2017
11. MarcoFalke added the label Needs backport on Sep 5, 2017
12. 
    TheBlueMatt commented at 9:07 PM on September 21, 2017: member

    Wait, do we want to also update if the witness was malleated on its way into a block?

13. 
    sdaftuar commented at 12:50 PM on September 22, 2017: member

    @TheBlueMatt I considered that issue, and decided it was more conservative (at least for now) to not do that, because: a) You can always figure out what version of the transaction got mined if you need to (just go look it up in the block file -- ok not great if you're pruning, but in theory this is possible) b) It's conceivable that the mined version is invalid-to-you (ie violates policy), and throwing away a known-good signature you made, which you might be able to reproduce, is scary, say in the event of a reorg that causes the malleated-version of the transaction to no longer be confirmed.

    b) is pretty edge-case, to be sure, but since the only benefit to storing the actually-confirmed-witness is recordkeeping, which doesn't affect our wallet's behavior, I figure why risk it. IMO we should eventually store both the original version of the transaction and any malleated versions that get confirmed.

14. 
    TheBlueMatt commented at 2:34 AM on September 25, 2017: member

    Yea, ok, more complexity than I figured, this is clearly better than nothing as-is and anything more looks like it'll take some real rewriting of our malleability handling. utACK d01a9682b126a5f83c7311e652e6e62f2c2e1d20 +/- outstanding nits.

15. laanwj merged this on Sep 26, 2017
16. laanwj closed this on Sep 26, 2017

    ---

17. laanwj referenced this in commit dc597bb895 on Sep 26, 2017
18. MarcoFalke referenced this in commit 6b4d9f2736 on Oct 4, 2017
19. MarcoFalke removed the label Needs backport on Oct 4, 2017
20. MarcoFalke locked this on Sep 8, 2021

Milestone
0.15.2