backport: bitcoin#15921, #16507, #16524, #16889, #16911, #17004, #17195, #17624, partial #17212 #5309

knst · 2023-04-10T08:44:30Z

Issue being fixed or feature implemented

Next batch of backports from bitcoin v20.

What was done?

bitcoin#17212 is partial because CExtPubKey serialization is used in our HD wallet implementation.

How Has This Been Tested?

Run unit/functional tests

Breaking Changes

Command argument -fallbackfee=<amt> is changed.

Backport bitcoin#17004 fixes minor bug in p2p behaviour:

Note that there is a minor bug fix in p2p behaviour here. Because the
call to MaybePunishNode() in PeerLogicValidation::BlockChecked() only
previously happened if the REJECT code was > 0 and < REJECT_INTERNAL,
then there are cases were MaybePunishNode() can get called where it
wasn't previously

Beside that, please, note, that:

instead of using CValidationState should be used TxValidationState for transactions and BlockValidationState for blocks.
the REJECT code is not kept anymore in state

Checklist:

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have assigned this pull request to a milestone

github-actions · 2023-04-14T20:32:00Z

This pull request has conflicts, please rebase.

UdjinM6

16524: it's incomplete - -fallbackfee wasn't disabled by default actually. 329e83f should improve it I think.

17004: should rename doc/release-notes-4892.md to doc/release-notes-17004.md

PastaPastaPasta · 2023-04-16T19:01:40Z

src/wallet/rpcwallet.cpp

@@ -2315,7 +2315,7 @@ static UniValue settxfee(const JSONRPCRequest& request)
 CAmount nAmount = AmountFromValue(request.params[0]);
 CFeeRate tx_fee_rate(nAmount, 1000);
 CFeeRate max_tx_fee_rate(pwallet->m_default_max_tx_fee, 1000);
- if (tx_fee_rate == 0) {


is there a reason why we don't have the p2p_feefilter.py test?

we don't have feefilter message

PastaPastaPasta · 2023-04-16T19:11:55Z

src/evo/deterministicmns.cpp

@@ -1451,8 +1451,9 @@ bool CheckProRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, TxVali
 return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-payload");
 }

- if (auto maybe_err = ptx.IsTriviallyValid(llmq::utils::IsV19Active(pindexPrev)); maybe_err.did_err) {
- return state.Invalid(maybe_err.reason, std::string(maybe_err.error_str));
+ if (!ptx.IsTriviallyValid(llmq::utils::IsV19Active(pindexPrev), state)) {


can we not mix backport and refactoring like this?

agree, 05757e6 is not needed PRs backported next to it, should be moved to a separate PR

UdjinM6

LGTM, utACK

PastaPastaPasta

utACK for merging via merge commit

PastaPastaPasta · 2023-04-17T15:41:57Z

Conflicting PRs: [#4956, #5167, #5026, #5109, #5236, #5306]

…stored rate eb7b781 modify p2p_feefilter test to catch rounding error (Gregory Sanders) 6a51f79 Disallow implicit conversion for CFeeRate constructor (Gregory Sanders) 8e59af5 feefilter: Compute the absolute fee rather than stored rate to match mempool acceptance logic (Gregory Sanders) Pull request description: This means we will use the rounding-down behavior in `GetFee` to match both mempool acceptance and wallet logic, with minimal changes. Fixes bitcoin#16499 Replacement PR for bitcoin#16500 ACKs for top commit: ajtowns: ACK eb7b781 code review only naumenkogs: utACK eb7b781 achow101: re ACK eb7b781 promag: ACK eb7b781. Tree-SHA512: 484a11c8f0e825f0c983b1f7e71cf6252b1bba6858194abfe4c088da3bae8a418ec539ef6c4181bf30940e277a95c08d493595d59dfcc6ddf77c65b05563dd7e

ea4cc3a Truly decouple wallet from chainparams for -fallbackfee (Jorge Timón) Pull request description: Before it was 0 by default for main and 20000 for test and regtest. Now it is 0 by default for all chains, thus there's no need to call Params(). Also now the default for main is properly documented. Suggestion for release notes: -fallbackfee was 0 (disabled) by default for the main chain, but 20000 by default for the test chains. Now it is 0 by default for all chains. Testnet and regtest users will have to add fallbackfee=20000 to their configuration if they weren't setting it and they want it to keep working like before. Should I propose them to the wiki for the release notes or only after merge? For more context, see bitcoin#16402 (comment) ACKs for top commit: MarcoFalke: ACK ea4cc3a Tree-SHA512: fdfaba5d813da4221e405e0988bef44f3856d10f897a94f9614386d14b7716f4326ab8a6646e26d41ef3f4fa61b936191e216b1b605e9ab0520b0657fc162e6c ---- Co-Authored-By: UdjinM6 <UdjinM6@users.noreply.github.com>

7d8d3e6 Add tests for util/vector.h's Cat and Vector (Pieter Wuille) e65e61c Add some general std::vector utility functions (Pieter Wuille) Pull request description: This is another general improvement extracted from bitcoin#16800 . Two functions are added are: * Vector(arg1,arg2,arg3,...) constructs a vector with the specified arguments as elements. The vector's type is derived from the arguments. If some of the arguments are rvalue references, they will be moved into place rather than copied (which can't be achieved using list initialization). * Cat(vector1,vector2) returns a concatenation of the two vectors, efficiently moving elements when relevant. Vector generalizes (and replaces) the `Singleton` function in src/descriptor.cpp, and `Cat` replaces the function in bech32.cpp ACKs for top commit: laanwj: ACK 7d8d3e6 MarcoFalke: ACK 7d8d3e6 (enjoyed reading the tests, but did not compile) Tree-SHA512: 92325f14e90d7e7d9d920421979aec22bb0d730e0291362b4326cccc76f9c2d865bec33a797c5c0201773468c3773cb50ce52c8eee4c1ec1a4d10db5cf2b9d2a

57e2ede Send amount shows minimum amount placeholder (JeremyCrookshank) Pull request description: Noticed that there wasn't a default value for the send amount. However if you put a value in or click the up and down arrows you're unable to get it blank again, so it makes sense that it has a default value. I hope this also makes it more clear that users can send less than 1 BTC if it shows the 8 decimal places PR: ![Capture](https://user-images.githubusercontent.com/46864828/67132088-549c6180-f1ff-11e9-9ba5-67fdcd6db894.PNG) ACKs for top commit: promag: ACK 57e2ede. GChuf: ACK 57e2ede laanwj: ACK 57e2ede, this is a surprisingly compact solution too Tree-SHA512: 354590d2a88231b8649f7ae985c8a7864d74ca0e1f8603cb1730ba46747084de90ee6285ce4d39ee04b054fb9cd2d78ebc71146f3af694c37a8a3aff7f051800

…ed from disk cd68594 Only check the hash of transactions loaded from disk (Andrew Chow) Pull request description: It feels unnecessary to do a full `CheckTransaction` for every transaction saved in the wallet. It should not be possible for an invalid transaction to get into the wallet in the first place, and if there is any disk corruption, the hash check will catch it. ACKs for top commit: MarcoFalke: ACK cd68594 laanwj: ACK cd68594 promag: ACK cd68594, AFAICT the check is not needed, hash comparison gives data integrity. Tree-SHA512: 5b2e719f76097cfbf125392db6cc6c764355c81f0b7a5b60aee4b06af1afcca80cfd38a3cf5307fd9e2c1afc405f8321929a4552943099a8161e6762965451fb

…tate 9075d13 [docs] Add release notes for removal of REJECT reasons (John Newbery) 04a2f32 [validation] Fix REJECT message comments (John Newbery) e9d5a59 [validation] Remove REJECT code from CValidationState (John Newbery) 0053e16 [logging] Don't log REJECT code when transaction is rejected (John Newbery) a1a07cf [validation] Fix peer punishment for bad blocks (John Newbery) Pull request description: We no longer send BIP 61 REJECT messages, so there's no need to set a REJECT code in the CValidationState object. Note that there is a minor bug fix in p2p behaviour here. Because the call to `MaybePunishNode()` in `PeerLogicValidation::BlockChecked()` only previously happened if the REJECT code was > 0 and < `REJECT_INTERNAL`, then there are cases were `MaybePunishNode()` can get called where it wasn't previously: - when `AcceptBlockHeader()` fails with `CACHED_INVALID`. - when `AcceptBlockHeader()` fails with `BLOCK_MISSING_PREV`. Note that `BlockChecked()` cannot fail with an 'internal' reject code. The only internal reject code was `REJECT_HIGHFEE`, which was only set in ATMP. This reverts a minor bug introduced in 5d08c9c. ACKs for top commit: ariard: ACK 9075d13, changes since last reviewed are splitting them in separate commits to ease understanding and fix nits fjahr: ACK 9075d13, confirmed diff to last review was fixing nits in docs/comments. ryanofsky: Code review ACK 9075d13. Only changes since last review are splitting the main commit and updating comments Tree-SHA512: 58e8a1a4d4e6f156da5d29fb6ad6a62fc9c594bbfc6432b3252e962d0e9e10149bf3035185dc5320c46c09f3e49662bc2973ec759679c0f3412232087cb8a3a7

3004d5a [validation] Remove fMissingInputs from AcceptToMemoryPool() (John Newbery) c428622 [validation] Remove unused first_invalid parameter from ProcessNewBlockHeaders() (John Newbery) 7204c64 [validation] Remove useless ret parameter from Invalid() (John Newbery) 1a37de4 [validation] Remove error() calls from Invalid() calls (John Newbery) 067981e [validation] Tidy Up ValidationResult class (John Newbery) a27a295 [validation] Add CValidationState subclasses (John Newbery) Pull request description: Carries out some remaining tidy-ups remaining after PR 15141: - split ValidationState into TxValidationState and BlockValidationState (commit from ajtowns) - various minor code style tidy-ups to the ValidationState class - remove the useless `ret` parameter from `ValidationState::Invalid()` - remove the now unused `first_invalid` parameter from `ProcessNewBlockHeaders()` - remove the `fMissingInputs` parameter from `AcceptToMemoryPool()`, and deal with missing inputs the same way as other errors by using the `TxValidationState` object. Tip for reviewers (thanks ryanofsky!): The first commit ("[validation] Add CValidationState subclasses" ) is huge and can be easier to start reviewing if you revert the rote, mechanical changes: Substitute the commit hash of commit "[validation] Add CValidationState subclasses" for <CommitHash> in the commands below. ```sh git checkout <CommitHash> git grep -l ValidationState | xargs sed -i 's/BlockValidationState\|TxValidationState/CValidationState/g' git grep -l ValidationResult | xargs sed -i 's/BlockValidationResult\|TxValidationResult/ValidationInvalidReason/g' git grep -l MaybePunish | xargs sed -i 's/MaybePunishNode$ForBlock\|ForTx$/MaybePunishNode/g' git diff HEAD^ ``` After that it's possible to easily see the mechanical changes with: ```sh git log -p -n1 -U0 --word-diff-regex=. <CommitHash> ``` ACKs for top commit: laanwj: ACK 3004d5a amitiuttarwar: code review ACK 3004d5a. Also built & ran tests locally. fjahr: Code review ACK 3004d5a . Only nit style change and pure virtual destructor added since my last review. ryanofsky: Code review ACK 3004d5a. Just whitespace change and pure virtual destructor added since last review. Tree-SHA512: 511de1fb380a18bec1944ea82b513b6192df632ee08bb16344a2df3c40811a88f3872f04df24bc93a41643c96c48f376a04551840fd804a961490d6c702c3d36

…(…, "tx", …) when receiving a transaction we already have 73b96c9 net: Fix uninitialized read in ProcessMessage(...) (practicalswift) Pull request description: Fix an uninitialized read in `ProcessMessage(…, "tx", …)` when receiving a transaction we already have. The uninitialized value is read and used on [L2526 in the case of `AlreadyHave(inv) == true`](https://github.com/bitcoin/bitcoin/blob/d8a66626d63135fd245d5afc524b88b9a94d208b/src/net_processing.cpp#L2494-L2526). Proof of concept being run against a `bitcoind` built with MemorySanitizer (`-fsanitize=memory`): ``` $ ./p2p-uninit-read-in-conditional-poc.py Usage: ./p2p-uninit-read-in-conditional-poc.py <dstaddr> <dstport> <net> $ bitcoind -regtest & $ ./p2p-uninit-read-in-conditional-poc.py 127.0.0.1 18444 regtest SUMMARY: MemorySanitizer: use-of-uninitialized-value [1]+ Exit 77 bitcoind -regtest $ ``` Proof of concept being run against a `bitcoind` running under Valgrind (`valgrind --exit-on-first-error`): ``` $ valgrind -q --exit-on-first-error=yes --error-exitcode=1 bitcoind -regtest & $ ./p2p-uninit-read-in-conditional-poc.py 127.0.0.1 18444 regtest ==27351== Conditional jump or move depends on uninitialised value(s) [1]+ Exit 1 valgrind -q --exit-on-first-error=yes --error-exitcode=1 bitcoind -regtest $ ``` Proof of concept script: ``` #!/usr/bin/env python3 import sys from test_framework.mininode import NetworkThread from test_framework.mininode import P2PDataStore from test_framework.messages import CTransaction, CTxIn, CTxOut, msg_tx def send_duplicate_tx(dstaddr="127.0.0.1", dstport=18444, net="regtest"): network_thread = NetworkThread() network_thread.start() node = P2PDataStore() node.peer_connect(dstaddr=dstaddr, dstport=dstport, net=net)() node.wait_for_verack() tx = CTransaction() tx.vin.append(CTxIn()) tx.vout.append(CTxOut()) node.send_message(msg_tx(tx)) node.send_message(msg_tx(tx)) node.peer_disconnect() network_thread.close() if __name__ == "__main__": if len(sys.argv) != 4: print("Usage: {} <dstaddr> <dstport> <net>".format(sys.argv[0])) sys.exit(0) send_duplicate_tx(sys.argv[1], int(sys.argv[2]), sys.argv[3]) ``` Note that the transaction in the proof of concept is the simplest possible, but really any transaction can be used. It does not have to be a valid transaction. This bug was introduced in bitcoin#15921 ("validation: Tidy up ValidationState interface") which was merged in to `master` 28 days ago. Luckily this bug was caught before being part of any Bitcoin Core release :) ACKs for top commit: jnewbery: utACK 73b96c9 laanwj: ACK 73b96c9, thanks for discovering and reporting this before it ended up in a release. Tree-SHA512: 7ce6b8f260bcdd9b2ec4ff4b941a891bbef578acf4456df33b7a8d42b248237ec4949e65e2445b24851d1639b10681c701ad500b1c0b776ff050ef8c3812c795

…e)serialization methods 5b44a75 refactor: Remove unused CExt{Pub,}Key (de)serialization methods (Sebastian Falbesoner) Pull request description: As pointed out in issue bitcoin#17130, the serialization/deserialization methods for the classes `CExtKey` and `CExtPubKey` are only used in the BIP32 unit tests and hence can be removed (see comments bitcoin#17130 (comment), bitcoin#17130 (comment) and bitcoin#17130 (comment)). ACKs for top commit: practicalswift: ACK 5b44a75 -- -60 LOC diff looks correct :) promag: ACK 5b44a75. MarcoFalke: unsigned ACK 5b44a75 fjahr: ACK 5b44a75 jonatack: Light ACK 5b44a75. Built, ran tests and bitcoind. `git blame` shows most of the last changes are from commit 90604f1 in 2015 to add bip32 pubkey serialization. Tree-SHA512: 6887573b76b9e54e117a076557407b6f7908719b2202fb9eea498522baf9f30198b3f78b87a62efcd17ad1ab0886196f099239992ce7cbbaee79979ffe9e5f2c

knst added this to the 20 milestone Apr 10, 2023

knst added the Needs Release Notes This PR includes breaking changes for which release notes have not yet been merged. label Apr 10, 2023

knst force-pushed the bc-bp-v20-missing-2 branch from 256ab15 to 2ef1dbf Compare April 10, 2023 09:55

knst requested review from UdjinM6 and PastaPastaPasta April 10, 2023 11:18

knst force-pushed the bc-bp-v20-missing-2 branch from 2ef1dbf to fe28408 Compare April 10, 2023 15:23

knst changed the title ~~backport: bitcoin#15921, #16507, #16524, #16889, #16911, #16973, #17004, #17195, #17624, partial #17212~~ backport: bitcoin#15921, #16507, #16524, #16889, #16911, #17004, #17195, #17624, partial #17212 Apr 10, 2023

knst force-pushed the bc-bp-v20-missing-2 branch from fe28408 to 6a6eda6 Compare April 10, 2023 16:30

github-actions bot added the needs rebase label Apr 14, 2023

knst force-pushed the bc-bp-v20-missing-2 branch from 6a6eda6 to 2cfa8bb Compare April 15, 2023 18:32

github-actions bot removed the needs rebase label Apr 15, 2023

UdjinM6 requested changes Apr 16, 2023

View reviewed changes

knst force-pushed the bc-bp-v20-missing-2 branch from 2cfa8bb to 8c2e7b1 Compare April 16, 2023 17:54

knst requested a review from UdjinM6 April 16, 2023 17:54

PastaPastaPasta reviewed Apr 16, 2023

View reviewed changes

knst force-pushed the bc-bp-v20-missing-2 branch from 8c2e7b1 to ef2f1a7 Compare April 17, 2023 06:03

knst requested a review from PastaPastaPasta April 17, 2023 06:04

UdjinM6 approved these changes Apr 17, 2023

View reviewed changes

PastaPastaPasta approved these changes Apr 17, 2023

View reviewed changes

fanquake and others added 9 commits April 17, 2023 10:42

PastaPastaPasta force-pushed the bc-bp-v20-missing-2 branch from ef2f1a7 to e2d1171 Compare April 17, 2023 15:42

PastaPastaPasta merged commit 03b0acd into dashpay:develop Apr 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

backport: bitcoin#15921, #16507, #16524, #16889, #16911, #17004, #17195, #17624, partial #17212 #5309

backport: bitcoin#15921, #16507, #16524, #16889, #16911, #17004, #17195, #17624, partial #17212 #5309

knst commented Apr 10, 2023 •

edited

Loading

github-actions bot commented Apr 14, 2023

UdjinM6 left a comment

PastaPastaPasta Apr 16, 2023

UdjinM6 Apr 16, 2023

PastaPastaPasta Apr 16, 2023

UdjinM6 Apr 16, 2023

UdjinM6 left a comment

PastaPastaPasta left a comment

PastaPastaPasta commented Apr 17, 2023

backport: bitcoin#15921, #16507, #16524, #16889, #16911, #17004, #17195, #17624, partial #17212 #5309

backport: bitcoin#15921, #16507, #16524, #16889, #16911, #17004, #17195, #17624, partial #17212 #5309

Conversation

knst commented Apr 10, 2023 • edited Loading

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

github-actions bot commented Apr 14, 2023

UdjinM6 left a comment

Choose a reason for hiding this comment

PastaPastaPasta Apr 16, 2023

Choose a reason for hiding this comment

UdjinM6 Apr 16, 2023

Choose a reason for hiding this comment

PastaPastaPasta Apr 16, 2023

Choose a reason for hiding this comment

UdjinM6 Apr 16, 2023

Choose a reason for hiding this comment

UdjinM6 left a comment

Choose a reason for hiding this comment

PastaPastaPasta left a comment

Choose a reason for hiding this comment

PastaPastaPasta commented Apr 17, 2023

knst commented Apr 10, 2023 •

edited

Loading