Skip to content

Conversation

@achow101
Copy link
Member

Backport of #21083

During each loop of CreateTransaction, instead of constantly getting a
new feerate, use the feerate that we have already fetched for all
fee calculations. Thix fixes a race condition where the feerate required
changes during each iteration of the loop.

This commit changes behavior as the "Fee estimation failed" error will
now take priority over "Signing transaction failed".

Github-Pull: bitcoin#21083
Rebased-From: 1a6a0b0
Make sure that all fee calculations use the same feerate.
coin_selection_params.effective_fee is the variable we use for all fee
calculations, so get rid of remaining nFeeRateNeeded usages and just
directly set coin_selection_params.effective_fee.

Does not change behavior.

Github-Pull: bitcoin#21083
Rebased-From: e2f429e
Instead of setting the long term feerate for each SelectCoinsMinConf
iteration, set it once during CreateTransaction and let it be shared
with each SelectCoinsMinConf through
coin_selection_params.m_long_term_feerate.

Does not change behavior.

Github-Pull: bitcoin#21083
Rebased-From: 448d04b
Instead of fetching the discard feerate for each SelectCoinsMinConf
iteration, fetch and cache it once during CreateTransaction so that it
is shared for each SelectCoinsMinConf through
coin_selection_params.m_discard_feerate.

Does not change behavior.

Github-Pull: bitcoin#21083
Rebased-From: bdd0c29
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cherry-pick-only ACK 4b7d19d 🐒

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

cherry-pick-only ACK 4b7d19d456a3a1e54bc6f24d4b4e7e8f417896df 🐒
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUil9wwAvjp9CweUIZcOEI6yjlm/XfbmPfpsNi20feQmD0jg/crBv4zwIRzdAY4L
zuzIhrJ9eG4ylMzQviuAtTS2UBx7BhQCBrMY3ZCNqZSS9m49zT/uhXdlXYXVacIG
prL/i1oL5J77jgxCrt3+zVC6XfO1BHXk8GgA2vWnIpI2MbNC3xS6ulZ2mbTosjsy
QlMv/0+H1qWLmK8Ce6t2S4EszyMHpsiatmIgJsLboIM1NpnwLPoXP7PFbJWW8wEx
eGe7yKd1UldUe6ryu9DOdcWsWNyZzHu9RHVlu5Q9UqUGF0CoXrdLQbC+kX1KcSE6
T0wMNdCN1udoDT3+uUxAtZJsBFbLKqQwhtZBvfHueZrVgAP5fIb9rABMvlBR2jOF
KGw287OdGzh46r/A+GC4sfZRRtol0gR9vr6TlfwU3m+tWO7LfO2YvdjQO6oX5YJS
9m1o4i/f8PEcae1L3SSTiGpZ92mcvNolk3Q+Yr9JfEvaGTmH5m4Rj00zJ7lG4pNG
GpufZO4/
=kp7c
-----END PGP SIGNATURE-----

Timestamp of file with hash 03e63e4b18da74fc9ba5be4a98a589d33a69777e74b0004659c5bb473a8cb037 -

It's a feerate, not a fee. Also follow the style guide for member names.

Github-Pull: bitcoin#21083
Rebased-From: f9cd2bf
@achow101 achow101 force-pushed the 0.21-createtx-same-feerate branch from 4b7d19d to d61fb07 Compare April 1, 2021 16:44
@maflcko
Copy link
Member

maflcko commented Apr 1, 2021

cherry-pick-only re-ACK d61fb07 🔙

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

cherry-pick-only re-ACK d61fb07da7c12e4a1f68cf645f32d563a657a506 🔙
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgBiwv+PHpjTYKRtUK3v9bYTZ2I0v6hzlydy0CNQAAm1XgJpy0moxairts8I8PL
Gi2dFUw7Utu/dFrucFjnr5SZjK7DyvehGnDSYuLjbNmQqinccmB02ulyrsheuGlT
izIpiku1EjqgBwYpyOg8qq8eAdQ/Ns9CJvnR+LUkXLhkkGr9YE44TcMDFCqJQKEb
y+g6V8J/zTnEnPiUmzeNYMVSTXhfROsblX3jFklb4AyjcvwqJExdq17vGq1aGQ3K
iC+vTmjaXzsliH+Y8x/MtRpf6Z3w8kEw09qGYD9lwSzFD6e0RhrbsEDePdhY/A+b
hv+Xtw25E4SFwpK88AOIhRowOzx6YdV9kO/o5VtaR6a0goqF8r9IF8GefJPzf6Z2
F5xo15FNz08k3sVpzwcaoZY275amIwVi51AoQwBSk4a0WmhQU/gbwFZ0x2puJ+uw
2xUw6VqD3QjGMQA+IefHnfmCULH7iM9nrzMgnxLG9J5Q1aAmHAo0I4DlM8DKolDW
aRM7tnY3
=Wqy6
-----END PGP SIGNATURE-----

Timestamp of file with hash da605e100922a3feb0ef43fceeae52979a9cfa9e2e57a77c72f8deef904034d7 -

@instagibbs
Copy link
Member

utACK d61fb07

@fanquake fanquake merged commit 0fe5b61 into bitcoin:0.21 Apr 16, 2021
@fjahr
Copy link
Contributor

fjahr commented Apr 16, 2021

Code review ACK d61fb07

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants