-maxapsfee follow-up by kallewoof · Pull Request #19743 · bitcoin/bitcoin

kallewoof · 2020-08-17T05:44:34Z

Addresses feedback from jonatack and meshcollider in #14582.

jonatack

Concept ACK! Do you plan to add @meshcollider's suggestion in #14582 (comment)?

doc/release-notes-14582.md

test/functional/wallet_groups.py

kallewoof · 2020-08-17T07:53:48Z

@jonatack Thanks for review! Added a test to check the cap is not exceeded. [Edited: Will → Did]

kallewoof · 2020-08-17T08:13:03Z

Added test for when cap is exceeded.

https://github.com/bitcoin/bitcoin/blob/452befa3999853383fc960d4982b244ab23d36b7/test/functional/wallet_groups.py#L114-L122

fjahr · 2020-08-17T12:22:02Z

Code review ACK 198c4434a2870c67e9b1b7b39800c10ec0404fb5

Edit: not sure about the CI failures, tests succeed for me locally.

jonatack

ACK 198c4434a28

jonatack · 2020-08-17T12:53:17Z

test/functional/wallet_groups.py

Verified the tests all pass if -maxapsfee=0.00002719, and that they fail at this line if -maxapsfee=0.00002720 (8240 - 5520 = 2720):

AssertionError: [node 3] Expected messages "['Fee non-grouped = 5520, grouped = 8240, using non-grouped']"

Actual log

Fee non-grouped = 5520, grouped = 8240, using grouped

So if you want to test the limit, could set maxapsfee to 0.00002719.

test/functional/wallet_groups.py

kallewoof · 2020-08-18T01:10:44Z

~~This should fix; if waiting for this to be merge-ready is undesired, #19756 was opened.~~

…ups test 72ae20f tests: add sync_all to fix race condition in wallet groups test (Karl-Johan Alm) Pull request description: This most likely fixes #19749, the intermittent CI issues with wallet_groups. This fix is also included in #19743. Top commit has no ACKs. Tree-SHA512: dd6ef7f89829483e2278191c21fe0912b51fd2187c10a0fa158339c5ab9f22d93b733ae10f17ef25d8b64f44e596e66dba8d7db5c009343472f422ce4cd67d8f

jonatack · 2020-08-18T06:49:24Z

ACK 19cf0d3 per git range-diff e6e277f9 198c443 19cf0d3 provided the CI agrees; the test passes for me locally

kallewoof · 2020-08-18T07:55:01Z

Weirdly, it's now giving a different message.

AssertionError: [node 3] Expected messages "['Fee non-grouped = 2820, grouped = 4160, using grouped']" does not partially match log:
[...]
2020-08-18T07:17:37.634268Z [httpworker.1] [default wallet] Fee non-grouped = 2820, grouped = 2820, using grouped

meshcollider · 2020-08-18T08:06:32Z

I get the same error locally ^

kallewoof · 2020-08-18T08:12:01Z

Rebase screw-up. Fixing.

jonatack · 2020-08-18T09:39:37Z

ACK 19cf0d3 per git range-diff e6e277f9 198c443 19cf0d3 provided the CI agrees; the test passes for me locally

I think I mistakenly ran the test while still on commit 198c443. Re-reviewing.

jonatack · 2020-08-18T09:40:49Z

test/functional/wallet_groups.py

+        tx5 = self.nodes[3].getrawtransaction(txid5, True)
+        # tx5 should have 3 inputs (1.0, 1.0, 1.0) and 2 outputs
+        assert_equal(3, len(tx5["vin"]))
+        assert_equal(2, len(tx4["vout"]))


Should this be tx5?

Suggested change

assert_equal(2, len(tx4["vout"]))

assert_equal(2, len(tx5["vout"]))

It should! Gah thanks.

meshcollider

functional test run ACK b740b76 modulo jon's comment (i.e. passes with his suggested change)

Btw Kalle, we edit out the '@' symbol tags in the OP because they get added to the merge commit and it spams users every time any other fork of the repo cherry-picks the PR :)

jonatack · 2020-08-18T10:12:10Z

@kallewoof further to #19743 (comment), here is a proposed additional test commit if you feel like pulling it in:

https://github.com/jonatack/bitcoin/commits/maxapsfee-tests

Co-authored-by: Jon Atack <[email protected]> Co-authored-by: Samuel Dobson <[email protected]>

kallewoof · 2020-08-18T10:27:11Z

Squashed @jonatack's improvements (including tx4 → tx5 typo) into main commit (already co-authored-by tagged).

jonatack · 2020-08-18T10:32:09Z

ACK 7e31ea9

(if Travis acks too)

meshcollider

re-ACK 7e31ea9

Test passes locally but I'll wait for Travis

…let groups test 72ae20f tests: add sync_all to fix race condition in wallet groups test (Karl-Johan Alm) Pull request description: This most likely fixes bitcoin#19749, the intermittent CI issues with wallet_groups. This fix is also included in bitcoin#19743. Top commit has no ACKs. Tree-SHA512: dd6ef7f89829483e2278191c21fe0912b51fd2187c10a0fa158339c5ab9f22d93b733ae10f17ef25d8b64f44e596e66dba8d7db5c009343472f422ce4cd67d8f

Summary: Co-authored-by: Jon Atack <[email protected]> Co-authored-by: Samuel Dobson <[email protected]> This is a backport of [[bitcoin/bitcoin#19743 | core#19743]] Notes: - the Release notes are not relevant because they refer to a PR that we backported almost one year ago. - the formula for computing a defaulti fee in Bitcoin ABC is `1 sat/byte * tx_size`, with `tx_size ~ 148 * n_in + 33 * n_out + 10`. The tx sizes in the log messages checked in this PR are consistent with this formula (and different from the way Core computes the fees). Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10094

kallewoof force-pushed the 20200817-maxapsfee-followup branch from 053af46 to 920006f Compare August 17, 2020 05:47

fanquake added the Wallet label Aug 17, 2020

jonatack reviewed Aug 17, 2020

View reviewed changes

doc/release-notes-14582.md Outdated Show resolved Hide resolved

doc/release-notes-14582.md Outdated Show resolved Hide resolved

test/functional/wallet_groups.py Outdated Show resolved Hide resolved

kallewoof force-pushed the 20200817-maxapsfee-followup branch from 920006f to e6547aa Compare August 17, 2020 07:52

kallewoof force-pushed the 20200817-maxapsfee-followup branch from e6547aa to 452befa Compare August 17, 2020 08:12

kallewoof force-pushed the 20200817-maxapsfee-followup branch from 452befa to 198c443 Compare August 17, 2020 10:04

jonatack reviewed Aug 17, 2020

View reviewed changes

jonatack mentioned this pull request Aug 17, 2020

Intermittent CI failure: wallet_groups.py #19749

Closed

jonatack reviewed Aug 17, 2020

View reviewed changes

test/functional/wallet_groups.py Outdated Show resolved Hide resolved

maflcko reviewed Aug 17, 2020

View reviewed changes

test/functional/wallet_groups.py Outdated Show resolved Hide resolved

kallewoof force-pushed the 20200817-maxapsfee-followup branch from 47a00be to e077f8c Compare August 18, 2020 01:07

kallewoof mentioned this pull request Aug 18, 2020

tests: add sync_all to fix race condition in wallet groups test #19756

Merged

kallewoof force-pushed the 20200817-maxapsfee-followup branch from e077f8c to 36cfd7f Compare August 18, 2020 05:30

doc: release notes for -maxapsfee

9f77b82

kallewoof force-pushed the 20200817-maxapsfee-followup branch from 36cfd7f to 19cf0d3 Compare August 18, 2020 06:31

kallewoof force-pushed the 20200817-maxapsfee-followup branch from 19cf0d3 to b740b76 Compare August 18, 2020 08:12

jonatack reviewed Aug 18, 2020

View reviewed changes

meshcollider reviewed Aug 18, 2020

View reviewed changes

-maxapsfee: follow-up fixes

7e31ea9

Co-authored-by: Jon Atack <[email protected]> Co-authored-by: Samuel Dobson <[email protected]>

kallewoof force-pushed the 20200817-maxapsfee-followup branch from b740b76 to 7e31ea9 Compare August 18, 2020 10:25

meshcollider reviewed Aug 18, 2020

View reviewed changes

meshcollider merged commit a2a250c into bitcoin:master Aug 18, 2020

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

	assert_equal(2, len(tx4["vout"]))
	assert_equal(2, len(tx5["vout"]))

Conversation

kallewoof commented Aug 17, 2020 • edited by meshcollider Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

kallewoof commented Aug 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Aug 17, 2020

Uh oh!

fjahr commented Aug 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Aug 17, 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!

Uh oh!

Uh oh!

kallewoof commented Aug 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Aug 18, 2020

Uh oh!

kallewoof commented Aug 18, 2020

Uh oh!

meshcollider commented Aug 18, 2020

Uh oh!

kallewoof commented Aug 18, 2020

Uh oh!

jonatack commented Aug 18, 2020

Uh oh!

jonatack Aug 18, 2020

Choose a reason for hiding this comment

Uh oh!

kallewoof Aug 18, 2020

Choose a reason for hiding this comment

Uh oh!

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

jonatack commented Aug 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Aug 18, 2020

Uh oh!

jonatack commented Aug 18, 2020

Uh oh!

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kallewoof commented Aug 17, 2020 •

edited by meshcollider

Loading

jonatack left a comment •

edited

Loading

kallewoof commented Aug 17, 2020 •

edited

Loading

fjahr commented Aug 17, 2020 •

edited

Loading

jonatack Aug 17, 2020 •

edited

Loading

kallewoof commented Aug 18, 2020 •

edited

Loading

meshcollider left a comment •

edited

Loading

jonatack commented Aug 18, 2020 •

edited

Loading

meshcollider left a comment •

edited

Loading