Test: wallet issue with orphaned rewards #18795

domob1812 · 2020-04-28T07:28:25Z

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

domob1812 · 2020-04-28T07:29:31Z

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

DrahtBot · 2020-04-28T08:35:32Z

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

Conflicts

No conflicts as of last run.

laanwj · 2020-04-30T13:54:29Z

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

domob1812 · 2020-05-01T07:12:04Z

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).

maflcko

Concept ACK

test/functional/wallet_orphanedreward.py

domob1812 · 2020-05-12T15:08:35Z

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.

maflcko

🄰🄲🄺 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 · 2020-05-14T04:56:53Z

@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).

maflcko · 2020-05-14T10:07:15Z

ACK 7aeaa92

decryp2kanon · 2020-10-04T17:07:11Z

Concept ACK

LarryRuane

ACK 7aeaa92
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.

test/functional/wallet_orphanedreward.py

LarryRuane · 2020-12-16T23:03:47Z

test/functional/wallet_orphanedreward.py

+          "untrusted_pending": 0,
+          "immature": 0,
+        })
+        self.nodes[1].sendtoaddress(address=self.nodes[0].getnewaddress(), amount=9, subtractfeefromamount=True)


Can this last sendtoaddress be removed?

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.

... 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.)

leonardojobim

tACK 7aeaa92. 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 (comment) and #18795 (comment). 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 · 2021-02-25T08:04:55Z

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 · 2021-02-25T18:42:41Z

ACK ef9f3f4

leonardojobim

reACK ef9f3f4

adamjonas · 2021-06-01T16:07:51Z

Ping for rebase @domob1812.

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

domob1812 · 2021-06-02T12:34:57Z

Thanks for the reminder, rebased.

LarryRuane

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

LarryRuane · 2021-06-02T21:05:09Z

test/functional/wallet_orphanedreward.py

+        self.nodes[0].sendtoaddress(self.nodes[1].getnewaddress(), 10)
+        self.nodes[0].generate(1)
+
+        # Get a block reward with node 1 and remember the block so we can orphan


Suggested change

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

# 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.)

adamjonas · 2021-06-03T16:04:42Z

CI fail is unrelated, see #20975

leonardojobim

reACK e4356f6 .
Tested on Ubuntu 20.04.

e4356f6 Testcase for wallet issue with orphaned rewards. (Daniel Kraft) Pull request description: This adds a new test case demonstrating the wallet issue when block rewards are orphaned (bitcoin#14148). ACKs for top commit: LarryRuane: ACK e4356f6 leonardojobim: reACK bitcoin@e4356f6 . Tree-SHA512: e9a2310ee1b3d52cfa302f431ed3d272bbc1b9195439ff318d9eb1006c0b28968dbe840e1600b6ff185e5d7ea57e4dcc837cef16051b5537445e10bc363b8c22

domob1812 mentioned this pull request Apr 28, 2020

abandontransaction needed after spending orphaned block reward #14148

Closed

fanquake added the Tests label Apr 28, 2020

DrahtBot mentioned this pull request Apr 28, 2020

[WIP] Index for UTXO Set Statistics #18000

Closed

maflcko reviewed May 11, 2020

View reviewed changes

maflcko approved these changes May 12, 2020

View reviewed changes

domob1812 force-pushed the orphanedreward-test branch 2 times, most recently from 957dee4 to 7aeaa92 Compare May 14, 2020 04:56

maflcko changed the title ~~Testcase for wallet issue with orphaned rewards~~ Test: wallet issue with orphaned rewards May 14, 2020

DrahtBot mentioned this pull request Jul 15, 2020

Coinstats Index #19521

Merged

DrahtBot mentioned this pull request Aug 24, 2020

rpc: Add dumpcoinstats #19792

Closed

LarryRuane reviewed Dec 16, 2020

View reviewed changes

leonardojobim approved these changes Feb 24, 2021

View reviewed changes

domob1812 force-pushed the orphanedreward-test branch from 7aeaa92 to ef9f3f4 Compare February 25, 2021 08:01

leonardojobim approved these changes Feb 26, 2021

View reviewed changes

DrahtBot mentioned this pull request Mar 2, 2021

test: add functional test for anchors.dat #21338

Merged

DrahtBot added the Needs rebase label Mar 24, 2021

Testcase for wallet issue with orphaned rewards.

e4356f6

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

domob1812 force-pushed the orphanedreward-test branch from ef9f3f4 to e4356f6 Compare June 2, 2021 12:33

DrahtBot removed the Needs rebase label Jun 2, 2021

LarryRuane approved these changes Jun 2, 2021

View reviewed changes

leonardojobim approved these changes Jun 3, 2021

View reviewed changes

maflcko merged commit 3ac5209 into bitcoin:master Jun 4, 2021

domob1812 deleted the orphanedreward-test branch June 4, 2021 14:23

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge 3ac5209 into merged_master (Bitcoin PR bitcoin/bitcoin#18795)

e4cbbd4

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

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

Test: wallet issue with orphaned rewards #18795

Test: wallet issue with orphaned rewards #18795

Uh oh!

Conversation

domob1812 commented Apr 28, 2020

Uh oh!

domob1812 commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

laanwj commented Apr 30, 2020

Uh oh!

domob1812 commented May 1, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

domob1812 commented May 12, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

domob1812 commented May 14, 2020

Uh oh!

maflcko commented May 14, 2020

Uh oh!

decryp2kanon commented Oct 4, 2020

Uh oh!

LarryRuane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

LarryRuane Dec 16, 2020 • 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!

domob1812 Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

LarryRuane Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

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

domob1812 commented Feb 25, 2021

Uh oh!

LarryRuane commented Feb 25, 2021

Uh oh!

leonardojobim left a comment

Choose a reason for hiding this comment

Uh oh!

adamjonas commented Jun 1, 2021

Uh oh!

domob1812 commented Jun 2, 2021

Uh oh!

LarryRuane left a comment

Choose a reason for hiding this comment

Uh oh!

LarryRuane Jun 2, 2021

Choose a reason for hiding this comment

Uh oh!

adamjonas commented Jun 3, 2021

Uh oh!

leonardojobim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

domob1812 commented Apr 28, 2020 •

edited

Loading

DrahtBot commented Apr 28, 2020 •

edited

Loading

LarryRuane Dec 16, 2020 •

edited

Loading

leonardojobim left a comment •

edited

Loading