Test: wallet issue with orphaned rewards #18795

domob1812 commented at 7:28 AM on April 28, 2020: contributor

This adds a new test case demonstrating the wallet issue when block rewards are orphaned (#14148).

domob1812 cross-referenced this on Apr 28, 2020 from issue abandontransaction needed after spending orphaned block reward by domob1812

fanquake added the label Tests on Apr 28, 2020

domob1812 commented at 7:29 AM on April 28, 2020: contributor

This is meant as an example for #14148 as requested by @MarcoFalke , not to be merged for now.

DrahtBot commented at 8:35 AM on April 28, 2020: contributor

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

Conflicts

No conflicts as of last run.

DrahtBot cross-referenced this on Apr 28, 2020 from issue [WIP] Index for UTXO Set Statistics by fjahr

laanwj commented at 1:54 PM on April 30, 2020: member

If this demonstrates an unfixed issue, shouldn't the test fail on master?

domob1812 commented at 7:12 AM on May 1, 2020: contributor

If this demonstrates an unfixed issue, shouldn't the test fail on master?

That's a good point. The test was originally written to assert the current behaviour and thus passed (but looking at the code one could see the behaviour described in #14148).

But I agree that is probably confusing. I've added another commit now that makes the test fail as expected (the original version is still preserved in the first commit, as it might be interesting as well for understanding what the issue is about).

in test/functional/wallet_orphanedreward.py:8 in 9f2ac53cbc outdated

   0 | @@ -0,0 +1,46 @@
   1 | +#!/usr/bin/env python3
   2 | +# Copyright (c) 2020 The Bitcoin Core developers
   3 | +# Distributed under the MIT software license, see the accompanying
   4 | +# file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5 | +"""Test orphaned block rewards in the wallet."""
   6 | +
   7 | +from test_framework.test_framework import BitcoinTestFramework
   8 | +from test_framework.util import assert_equal, assert_raises_rpc_error

MarcoFalke commented at 10:53 PM on May 11, 2020:

test/functional/wallet_orphanedreward.py:8:1: F401 'test_framework.util.assert_raises_rpc_error' imported but unused

domob1812 commented at 3:05 PM on May 12, 2020:

Indeed, that's a left over from the previous commit. Removed.

in test/functional/wallet_orphanedreward.py:14 in 9f2ac53cbc outdated

   9 | +
  10 | +class OrphanedBlockRewardTest(BitcoinTestFramework):
  11 | +    def set_test_params(self):
  12 | +        self.setup_clean_chain = True
  13 | +        self.num_nodes = 2
  14 | +        self.supports_cli = False

MarcoFalke commented at 10:53 PM on May 11, 2020:

Why is this needed?

domob1812 commented at 3:05 PM on May 12, 2020:

Removed.

in test/functional/wallet_orphanedreward.py:12 in 9f2ac53cbc outdated

   7 | +from test_framework.test_framework import BitcoinTestFramework
   8 | +from test_framework.util import assert_equal, assert_raises_rpc_error
   9 | +
  10 | +class OrphanedBlockRewardTest(BitcoinTestFramework):
  11 | +    def set_test_params(self):
  12 | +        self.setup_clean_chain = True

MarcoFalke commented at 10:53 PM on May 11, 2020:

For a speed-up you can remove this line, and then start the test with sendtoaddress.

domob1812 commented at 3:06 PM on May 12, 2020:

I think that a clean chain makes the test easier to read, as it doesn't require any prior knowledge about how the cached chain looks like. If we use the cached chain, it will be harder to make sure that node[1] only has the coins we want to, as it will have immature coins that will appear over time.

Also, it seems the initial setup (including until after the sendtoaddress) is quick anyway. The slow parts are the sync_blocks calls, which we wouldn't get rid of with a cached chain.

Or did I misunderstand your suggestion?

in test/functional/wallet_orphanedreward.py:58 in 9f2ac53cbc outdated

  38 | +        # Orphan the block reward and make sure that the original coins
  39 | +        # from the wallet can still be spent.
  40 | +        self.nodes[0].invalidateblock(blk)
  41 | +        self.nodes[0].generate(200)
  42 | +        self.sync_blocks()
  43 | +        self.nodes[1].sendtoaddress(self.nodes[0].getnewaddress(), 9)

MarcoFalke commented at 11:16 PM on May 11, 2020:

Could comment this out, and add a comment that it fails?

MarcoFalke commented at 11:16 PM on May 11, 2020:

Also, would be good to check the output of getbalances here.

domob1812 commented at 3:07 PM on May 12, 2020:

Instead of this, I've now added in the abandontransaction call that makes the test succeed (but according to #14148 should not be needed) as well as a comment explaining this. I think that makes the situation even more clear.

domob1812 commented at 3:07 PM on May 12, 2020:

Added.

MarcoFalke commented at 11:17 PM on May 11, 2020: member

Concept ACK

domob1812 commented at 3:08 PM on May 12, 2020: contributor

Thanks for the feeback, @MarcoFalke ! I've addressed it with a new commit (so as to not invalidate comments made on previous commits), but am of course happy to squash all history into a single commit any time.

MarcoFalke approved

MarcoFalke commented at 3:14 PM on May 12, 2020: member

🄰🄲🄺 I think I am going to merge this.

Maybe squash the commits and add a check for getbalances also before the abandontransaction call. Due to the bug it should be all zeros, right?

domob1812 force-pushed on May 14, 2020

domob1812 commented at 4:56 AM on May 14, 2020: contributor

@MarcoFalke Thanks! I've rebased onto latest master, squashed all commits, and added in a getbalances check before the abandontransaction call as well (which indeed shows all zero).

MarcoFalke renamed this:
~~Testcase for wallet issue with orphaned rewards~~
Test: wallet issue with orphaned rewards
on May 14, 2020

MarcoFalke commented at 10:07 AM on May 14, 2020: member

ACK 7aeaa92ae0d45cddd2cdbdc735cdbcfa5fb2871a

DrahtBot cross-referenced this on Jul 15, 2020 from issue Coinstats Index by fjahr

DrahtBot cross-referenced this on Aug 24, 2020 from issue rpc: Add dumpcoinstats by fjahr

decryp2kanon commented at 5:07 PM on October 4, 2020: contributor

Concept ACK

in test/functional/wallet_orphanedreward.py:35 in 7aeaa92ae0 outdated

  30 | +
  31 | +        # Let the block reward mature and send coins including both
  32 | +        # the existing balance and the block reward.
  33 | +        self.nodes[0].generate(150)
  34 | +        assert_equal(self.nodes[1].getbalance(), 10 + 25)
  35 | +        txid = self.nodes[1].sendtoaddress(address=self.nodes[0].getnewaddress(), amount=30, subtractfeefromamount=True)

LarryRuane commented at 10:46 PM on December 16, 2020:

        txid = self.nodes[1].sendtoaddress(self.nodes[0].getnewaddress(), 30)

domob1812 commented at 8:01 AM on February 25, 2021:

Indeed. Not sure why I had the other form; maybe a previous version of the test needed the subtractfeefromamount, but that is no longer the case.

in test/functional/wallet_orphanedreward.py:40 in 7aeaa92ae0 outdated

  35 | +        txid = self.nodes[1].sendtoaddress(address=self.nodes[0].getnewaddress(), amount=30, subtractfeefromamount=True)
  36 | +
  37 | +        # Orphan the block reward and make sure that the original coins
  38 | +        # from the wallet can still be spent.
  39 | +        self.nodes[0].invalidateblock(blk)
  40 | +        self.nodes[0].generate(200)

LarryRuane commented at 11:02 PM on December 16, 2020:

I wasn't sure why this generate(200) was needed, so I commented it out, and the test hung in the next line (sync_blocks()). I restored the line and played around with different generate counts, and found that it fails with 151 and works at 152. If I'm counting correctly, we invalidated 151 blocks, so it seems that it's necessary to extend the chain so it's longer than it was before. (Necessary for this test framework, not in real life.)

You could reduce this to 152; I like tests to "just barely" pass because it seems like that imparts better understanding to readers (I wondered is, "why is this 200?"), and also because if the code is acting differently from our understanding, for example, we think generating 152 blocks should work but it actually takes 170, and the test does 200, then that mistaken understanding (or even a bug in the code) would be papered over.

Also, the test runs a little faster if we reduce this.

Of course, a downside to tests "just barely" passing is that they're more fragile. It's painful when someone makes a perfectly good change and one of the tests break! (I don't think there's much of a chance of that happening here.)

domob1812 commented at 8:02 AM on February 25, 2021:

Sounds good to me, updated to 152.

in test/functional/wallet_orphanedreward.py:58 in 7aeaa92ae0 outdated

  53 | +        assert_equal(self.nodes[1].getbalances()["mine"], {
  54 | +          "trusted": 10,
  55 | +          "untrusted_pending": 0,
  56 | +          "immature": 0,
  57 | +        })
  58 | +        self.nodes[1].sendtoaddress(address=self.nodes[0].getnewaddress(), amount=9, subtractfeefromamount=True)

LarryRuane commented at 11:03 PM on December 16, 2020:

Can this last sendtoaddress be removed?

domob1812 commented at 8:02 AM on February 25, 2021:

Yes it can, but I think having it in there is an extra check that the wallet does properly handle the coins. So unless there is a strong preference to remove it, I'd rather keep it in to make the test more robust and descriptive.

LarryRuane commented at 6:42 PM on February 25, 2021:

... having it in there is an extra check ...

I verified that if this sendtoaddress fails (which I forced by increasing the amount sent to 99999), the entire test fails, which is good. (I wasn't aware of that, which is why I thought this was an unnecessary line of code -- good to leave it in as you have. Need to put an RPC within a try to have its failure not fail the overall test.)

LarryRuane commented at 11:07 PM on December 16, 2020: contributor

ACK 7aeaa92ae0d45cddd2cdbdc735cdbcfa5fb2871a Nice PR! I made sure the test passes as written, and that removing the abandontransaction makes it fail as expected. Suggestions are minor nits, nonblocking.

leonardojobim approved

leonardojobim commented at 4:52 AM on February 24, 2021: none

tACK https://github.com/bitcoin/bitcoin/pull/18795/commits/7aeaa92ae0d45cddd2cdbdc735cdbcfa5fb2871a. Tested on Ubuntu 20.04.

The test successfully demonstrates that when an orphaned block reward is spent together with other outputs, the wallet stops showing any funds and that "abandontransaction(txid)" restores the valid funds.

Agree with #18795 (review) and #18795 (review). Using 152 blocks apparently works fine and there's no need of the last line.

The code below would also work as the final validation of this test:

        self.stop_node(1)
        self.start_node(1, ["-zapwallettxes=2"])

        assert_equal(self.nodes[1].getbalances()["mine"], {
          "trusted": 10,
          "untrusted_pending": 0,
          "immature": 0,
        })

domob1812 force-pushed on Feb 25, 2021

domob1812 commented at 8:04 AM on February 25, 2021: contributor

Thanks @LarryRuane and @leonardojobim , and sorry for the long response time on my end. I have now rebased the PR onto latest master and addressed your comments.

As for the alternate final validation with -zapwallettxes, I agree this is possible, but I think abandontransaction is more explicit as it specifies the transaction in question. I'm happy to change it, though.

LarryRuane commented at 6:42 PM on February 25, 2021: contributor

ACK ef9f3f4b25a6158aa3ff32bf74c104c325c910b1

leonardojobim approved

leonardojobim commented at 12:21 AM on February 26, 2021: none

reACK https://github.com/bitcoin/bitcoin/pull/18795/commits/ef9f3f4b25a6158aa3ff32bf74c104c325c910b1

DrahtBot cross-referenced this on Mar 2, 2021 from issue test: add functional test for anchors.dat by brunoerg

DrahtBot added the label Needs rebase on Mar 24, 2021

adamjonas commented at 4:07 PM on June 1, 2021: member

Ping for rebase @domob1812.

Testcase for wallet issue with orphaned rewards.

This adds a new test case demonstrating the wallet issue when block
rewards are orphaned (https://github.com/bitcoin/bitcoin/issues/14148).

e4356f6a6c

domob1812 force-pushed on Jun 2, 2021

domob1812 commented at 12:34 PM on June 2, 2021: contributor

Thanks for the reminder, rebased.

DrahtBot removed the label Needs rebase on Jun 2, 2021

in test/functional/wallet_orphanedreward.py:25 in e4356f6a6c

  20 | +        # some balance to node 1, which will hold it as a single coin.
  21 | +        self.nodes[0].generate(150)
  22 | +        self.nodes[0].sendtoaddress(self.nodes[1].getnewaddress(), 10)
  23 | +        self.nodes[0].generate(1)
  24 | +
  25 | +        # Get a block reward with node 1 and remember the block so we can orphan

LarryRuane commented at 9:05 PM on June 2, 2021:

        # Get a block reward (25) with node 1 and remember the block so we can orphan

(Non-blocking, only if you touch-up, would have made the test easier for me to understand.)

LarryRuane approved

LarryRuane commented at 9:09 PM on June 2, 2021: contributor

ACK e4356f6a6c18e5027a064a4d3a5deda27985f584 I verified that without the abandontransaction, the assertion fails, and (if I comment out the assertion) the sendtoaddress fails.

adamjonas commented at 4:04 PM on June 3, 2021: member

CI fail is unrelated, see https://github.com/bitcoin/bitcoin/issues/20975

leonardojobim approved

leonardojobim commented at 10:43 PM on June 3, 2021: none

reACK https://github.com/bitcoin/bitcoin/pull/18795/commits/e4356f6a6c18e5027a064a4d3a5deda27985f584 . Tested on Ubuntu 20.04.

MarcoFalke merged this on Jun 4, 2021

MarcoFalke closed this on Jun 4, 2021

domob1812 deleted the branch on Jun 4, 2021

sidhujag referenced this in commit d3e62a7199 on Jun 4, 2021

gwillen referenced this in commit e4cbbd4372 on Jun 1, 2022

bitcoin locked this on Aug 18, 2022