The migration process reloads the wallet after all failures.
This commit tests the behavior by trying to obtain a new address
after a decryption failure during migration.
Convert XoRoShiRo128PlusPlus into a full RandomMixin-based RNG class,
providing all utility functionality that FastRandomContext has. In doing so,
it is renamed to InsecureRandomContext, highlighting its non-cryptographic
nature.
To do this, a fillrand fallback is added to RandomMixin (where it is used by
InsecureRandomContext), but FastRandomContext still uses its own fillrand.
In many cases, it is known at compile time how many bits are requested from
randbits. Provide a variant of randbits that accepts this number as a template,
to make sure the compiler can make use of this knowledge. This is used immediately
in rand32() and randbool(), and a few further call sites.
The previous randbits code would, when requesting more randomness than available
in its random bits buffer, discard the remaining entropy and generate new.
Benchmarks show that it's usually better to first consume the existing randomness
and only then generate new ones. This adds some complexity to randbits, but it
doesn't weigh up against the reduced need to generate more randomness.
The "connect to ourself" detection logic has been first introduced
by Satoshi in October 2009, together with a couple of other changes
and a version bump to "v0.1.6 BETA" (see commit
cc0b4c3b62).
73f0a6cbd0 doc: detail -rpccookieperms option (willcl-ark)
d2afa2690c test: add rpccookieperms test (willcl-ark)
f467aede78 init: add option for rpccookie permissions (willcl-ark)
7df03f1a92 util: add perm string helper functions (willcl-ark)
Pull request description:
This PR picks up #26088 by aureleoules which adds a bitcoind launch option `-rpccookieperms` to set the file permissions of the cookie generated by bitcoin core.
Example usage to make the generated cookie group-readable: `./src/bitcoind -rpccookieperms=group`.
Accepted values for `-rpccookieperms` are `[owner|group|all]`. We let `fs::perms` handle platform-specific permissions changes.
ACKs for top commit:
achow101:
ACK 73f0a6cbd0
ryanofsky:
Code review ACK 73f0a6cbd0. Main change since last review is no longer throwing a skip exception in the rpc test on windows, so other checks can run after it, and overall test result is passing, not skipped. Also were clarifying renames and documentation improvements.
tdb3:
cr ACK 73f0a6cbd0
Tree-SHA512: e800d59a44aca10e1c58ca69bf3fdde9f6ccf5eab4b7b962645af6d6bc0cfa3a357701e409c8c60d8d7744fcd33a91e77ada11790aa88cd7811ef60fab86ab11
This allows a transaction's weight to be bound under a certain
weight if possible and desired. This can be beneficial for future
RBF attempts, or whenever a more restricted spend topology is
desired.
Co-authored-by: Greg Sanders <gsanders87@gmail.com>
72b226882f wallet: notify when preset + automatic inputs exceed max weight (furszy)
Pull request description:
Small change. Found it while finishing my review on #29523. This does not interfere with it.
Basically, we are erroring out early when the automatic coin selection process exceeds the maximum weight, but we are not doing so when the user-preselected inputs combined with the wallet-selected inputs exceed the maximum weight.
This change avoids signing all inputs before erroring out and introduces test coverage for `fundrawtransaction`.
ACKs for top commit:
achow101:
ACK 72b226882f
tdb3:
re ACK for 72b226882f
rkrux:
tACK [72b2268](72b226882f)
ismaelsadeeq:
utACK 72b226882f
Tree-SHA512: d77be19231023383a9c79a5d66b642dcbc6ebfc31a363e0b9f063c44898720a7859ec211cdbc0914ac7a3bfdf15e52fb8fc20d97f171431f70492c0f159dbc36
a9716c53f0 rpc: call IsInitialBlockDownload via miner interface (Sjors Provoost)
dda0b0834f rpc: minize getTipHash() calls in gbt (Sjors Provoost)
7b4d3249ce rpc: call processNewBlock via miner interface (Sjors Provoost)
9e228351e7 rpc: getTransactionsUpdated via miner interface (Sjors Provoost)
64ebb0f971 Always pass options to BlockAssembler constructor (Sjors Provoost)
4bf2e361da rpc: call CreateNewBlock via miner interface (Sjors Provoost)
404b01c436 rpc: getblocktemplate getTipHash() via Miner interface (Sjors Provoost)
d8a3496b5a rpc: call TestBlockValidity via miner interface (Sjors Provoost)
8ecb681678 Introduce Mining interface (Sjors Provoost)
Pull request description:
Introduce a `Mining` interface for the `getblocktemplate`, `generateblock` and other mining RPCs to use now, and for Stratum v2 to use later.
Suggested here: https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2108528652
The selection of methods added to the interface is mostly based on what the Template Provider in #29432 uses. It could be expanded further so that `rpc/mining.cpp` no longer needs `EnsureMemPool` and `EnsureChainman`.
This PR should be a pure refactor.
ACKs for top commit:
tdb3:
re ACK a9716c53f0
itornaza:
Code review and std-tests ACK a9716c53f0
ryanofsky:
Code review ACK a9716c53f0 with one minor suggestion in case you update. Only changes since last review were other small changes to the interface.
Tree-SHA512: cf97f87d6e9ed89da3835a0730da3b24a7b14c8605ea221149103a5915e79598cf082a95f2bc88e33f1c450e3d4aad88aed1163a29195acca88bcace055af724
This test type is represented using SEND_NO_AAD. If AAD of the first encrypted packet
sent after the garbage terminator (optional decoy packet/version packet) hasn't been
filled, disconnection happens.
This test type is represented using WRONG_GARBAGE.
Here, garbage bytes sent to TestNode are assumed to be tampered with and
do not correspond to the garbage bytes which P2PInterface calculated and
uses.
This test type is represented using WRONG_GARBAGE_TERMINATOR.
since the wrong garbage terminator is sent to TestNode, TestNode
will interpret all of the gabage bytes, wrong garbage terminator,
decoy messages and version packet it receives as garbage bytes.
If the length of all these is more than 4095 + 16, it will result
in a missing garbage terminator error. otherwise, it will result
in a V2 handshake timeout error.
Send only MAX_GARBAGE_LEN//2 bytes of garbage data to TestNode
so that the total length received by the TestNode is at max
= (MAX_GARBAGE_LEN//2) + 16 + 10*120 + 20 = 3283 bytes
(which is less than 4095 + 16 bytes) and we get a consistent
V2 handshake timeout error message.
If we do not limit the garbage length sent, we will intermittently
get both missing garbage terminator error and V2 handshake
timeout error based on the garbage length and decoy packets length
which are chosen at random.
6eecba475e net_processing: make MaybePunishNodeFor{Block,Tx} return void (Pieter Wuille)
ae60d485da net_processing: remove Misbehavior score and increments (Pieter Wuille)
6457c31197 net_processing: make all Misbehaving increments = 100 (Pieter Wuille)
5120ab1478 net_processing: drop 8 headers threshold for incoming BIP130 (Pieter Wuille)
944c54290d net_processing: drop Misbehavior for unconnecting headers (Pieter Wuille)
9f66ac7cf1 net_processing: do not treat non-connecting headers as response (Pieter Wuille)
Pull request description:
So far, discouragement of peers triggers when their misbehavior score exceeds 100 points. Most types of misbehavior increment the score by 100, triggering immediate discouragement, but some types do not. This PR makes all increments equal to either 100 (meaning any misbehavior will immediately cause disconnection and discouragement) or 0 (making the behavior effectively unconditionally allowed), and then removes the logic for score accumulation.
This simplifies the code a bit, but also makes protocol expectations clearer: if a peer misbehaves, they get disconnected. There is no good reason why certain types of protocol violations should be permitted 4 times (howmuch=20) or 9 times (howmuch=10), while many others are never allowed. Furthermore, the distinction between these looks arbitrary.
The specific types of misbehavior that are changed to 100 are:
* Sending us a `block` which does not connect to our header tree (which necessarily must have been unsollicited). [used to be score 10]
* Sending us a `headers` with a non-continuous headers sequence. [used to be score 20]
* Sending us more than 1000 addresses in a single `addr` or `addrv2` message [used to be score 20]
* Sending us more than 50000 invs in a single `inv` message [used to be score 20]
* Sending us more than 2000 headers in a single `headers` message [used to be score 20]
The specific types of misbehavior that are changed to 0 are:
* Sending us 10 (*) separate BIP130 headers announcements that do not connect to our block tree [used to be score 20]
* Sending us more than 8 headers in a single `headers` message (which thus does not get treated as a BIP130 announcement) that does not connect to our block tree. [used to be score 10]
I believe that none of these behaviors are unavoidable, except for the one marked (*) which can in theory happen still due to interaction between BIP130 and variations in system clocks (the max 2 hour in the future rule). This one has been removed entirely. In order to remove the impact of the bug it was designed to deal with, without relying on misbehavior, a separate improvement is included that makes `getheaders`-tracking more accurate.
In another unrelated improvement, this also gets rid of the 8 header limit heuristic to determine whether an incoming non-connecting `headers` is a potential BIP130 announcement, as this rule is no longer needed to prevent spurious Misbehavior. Instead, any non-connecting `headers` is now treated as a potential announcement.
ACKs for top commit:
sr-gi:
ACK [6eecba4](6eecba475e)
achow101:
ACK 6eecba475e
mzumsande:
Code Review ACK 6eecba475e
glozow:
light code review / concept ACK 6eecba475e
Tree-SHA512: e11e8a652c4ec048d8961086110a3594feefbb821e13f45c14ef81016377be0db44b5311751ef635d6e026def1960aff33f644e78ece11cfb54f2b7daa96f946
94ed4fbf8e Add release note for size 2 package rbf (Greg Sanders)
afd52d8e63 doc: update package RBF comment (Greg Sanders)
6e3c4394cf mempool: Improve logging of replaced transactions (Greg Sanders)
d3466e4cc5 CheckPackageMempoolAcceptResult: Check package rbf invariants (Greg Sanders)
316d7b63c9 Fuzz: pass mempool to CheckPackageMempoolAcceptResult (Greg Sanders)
4d15bcf448 [test] package rbf (glozow)
dc21f61c72 [policy] package rbf (Suhas Daftuar)
5da3967815 PackageV3Checks: Relax assumptions (Greg Sanders)
Pull request description:
Allows any 2 transaction package with no in-mempool ancestors to do package RBF when directly conflicting with other mempool clusters of size two or less.
Proposed validation steps:
1) If the transaction package is of size 1, legacy rbf rules apply.
2) Otherwise the transaction package consists of a (parent, child) pair with no other in-mempool ancestors (or descendants, obviously), so it is also going to create a cluster of size 2. If larger, fail.
3) The package rbf may not evict more than 100 transactions from the mempool(bip125 rule 5)
4) The package is a single chunk
5) Every directly conflicted mempool transaction is connected to at most 1 other in-mempool transaction (ie the cluster size of the conflict is at most 2).
6) Diagram check: We ensure that the replacement is strictly superior, improving the mempool
7) The total fee of the package, minus the total fee of what is being evicted, is at least the minrelayfee * size of the package (equivalent to bip125 rule 3 and 4)
Post-cluster mempool this will likely be expanded to general package rbf, but this is what we can safely support today.
ACKs for top commit:
achow101:
ACK 94ed4fbf8e
glozow:
reACK 94ed4fbf8e via range-diff
ismaelsadeeq:
re-ACK 94ed4fbf8e
theStack:
Code-review ACK 94ed4fbf8e
murchandamus:
utACK 94ed4fbf8e
Tree-SHA512: 9bd383e695964f362f147482bbf73b1e77c4d792bda2e91d7f30d74b3540a09146a5528baf86854a113005581e8c75f04737302517b7d5124296bd7a151e3992
881724d443 test: Added test coverage to listsinceblock rpc (kevkevinpal)
Pull request description:
This change is meant to add test coverage to this rpc error https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpc/transactions.cpp#L666C53-L666C79
This is done by renaming the first block in the blocks folder
---
Doing a quick grep for the error code in our functional tests leads to zero results
`grep -nri "Can't read block from disk" ./test/functional/`
ACKs for top commit:
achow101:
ACK 881724d443
tdb3:
re ACK for 881724d443
rkrux:
tACK [881724](881724d443)
Tree-SHA512: c5dff20cf014d0181f49d6b161f1364e1c6b79e8661047f77f07e21e59f4d1f2fd6f745538c8fc5bd6d4244650a840dd64d184634366f7c21fa67141a60af44a
5cf0a1f230 test: add `createmultisig` P2MS encoding test for all n (1..20) (Sebastian Falbesoner)
0570d2c204 test: add unit test for `keys_to_multisig_script` (Sebastian Falbesoner)
0c41fc3fa5 test: fix `keys_to_multisig_script` (P2MS) helper for n/k > 16 (Sebastian Falbesoner)
Pull request description:
While reviewing #28307, I noticed that the test framework's `key_to_multisig_script` helper (introduced in #23305) is broken for pubkey count (n) and threshold (k) values larger than 16. This is due to the implementation currently enforcing a direct single-byte data push (using `CScriptOp.encode_op_n`), which obviously fails for values 17+. Fix that by passing the numbers directly to the CScript list, where it's automatically converted to minimally-encoded pushes (see class method `CScript.__coerce_instance`, branch `isinstance(other, int)`).
The second commit adds a unit test to ensure that the encoding is correct.
ACKs for top commit:
achow101:
ACK 5cf0a1f230
tdb3:
ACK 5cf0a1f230
rkrux:
reACK [5cf0a1f](5cf0a1f230)
Tree-SHA512: 4168a165c3f483ec8e37a27dba1628a7ea0063545a2b7e74d9e20d753fddd7e33d37e1a190434fa6dca39adf9eef5d0211f7a0c1c7b44979f0a3bb350e267562
Prior to this commit, TestEncryptedP2PState would always
send initial_v2_handshake bytes in 2 parts (as required
by early key response test).
For generalising this test and having different v2 handshake
behaviour based on the test type, special behaviours like
sending initial_v2_handshake bytes in 2 parts are executed
only if test_type is set to EARLY_KEY_RESPONSE.
Adds argument --resultsfile to test_runner.py.
Writes comma-separated functional test name, status,
and duration to the file provided with the argument.
Also fixes minor typo in test_runner.py
f58beabe75 test: bumpfee with user specified fee_rate ignores walletIncrementalRelayFee (ismaelsadeeq)
436e88f433 bumpfee: ignore WALLET_INCREMENTAL_RELAY_FEE when user specifies fee rate (ismaelsadeeq)
Pull request description:
Fixes#26973
When using the `bumpfee` RPC and manually specifying `fee_rate`, there should be no requirement that the new fee must be at least the sum of the original fee and `incrementalFee` (maximum of `relayIncrementalFee` and `WALLET_INCREMENTAL_RELAY_FEE`).
This restriction should only apply when user did not specify `fee_rate`.
> because the GUI doesn't let the user specify the new fee rate yet (https://github.com/bitcoin-core/gui/issues/647), it would be very annoying to have to bump 20 times to increment by 20 sat/vbyte.
The restriction should instead be the new fee must be at least the sum of the original fee and `incrementalFee` (`relayIncrementalFee`)
ACKs for top commit:
achow101:
ACK f58beabe75
murchandamus:
ACK f58beabe75
Tree-SHA512: 193259f87173b7d5a8e68e0e29f2ca7e75c550e3cf0dee3d6d822b5b1e07c2e6dec0bfc8fb435855736ebced97a10dbdbfef72e8c5abde06fdefcba122f2e7f1
e2779ce98b test: cover more errors for `signrawtransactionwithkey` RPC (brunoerg)
Pull request description:
This PR adds test coverage for the following errors for the `signrawtransactionwithkey` RPC:
- Invalid private key
- TX decode failed
For reference: https://maflcko.github.io/b-c-cov/total.coverage/src/rpc/rawtransaction.cpp.gcov.html
ACKs for top commit:
maflcko:
ACK e2779ce98b
kevkevinpal:
ACK [e2779ce](e2779ce98b)
tdb3:
ACK e2779ce98b
BrandonOdiwuor:
Code Review ACK e2779ce98b
Tree-SHA512: 41c7e990684b60645cf4ccec8aad5ebbe61da221871eb3c1685b2bb1eebda58b29358502cb1525b7c7a2b612e2bebf449ed0bae14ab663b4641c528a9c013b5b
0fcbfdb7ad Support running individual lint checks (David Gumberg)
Pull request description:
This PR was split out from #29965:
Adds support for running individual tests in the rust lint suite by passing `--lint=LINT_TO_RUN` to the lint runner. This PR also adds a corresponding help message.
When running with `cargo run`, arguments after a double dash (`--`) are passed to the binary instead of the cargo command. For example, in order to run the linter check that tabs are not used as whitespace:
```console
cd test/lint/test_runner && cargo run -- --lint=tabs_whitespace
```
ACKs for top commit:
maflcko:
ACK 0fcbfdb7ad
achow101:
ACK 0fcbfdb7ad
marcofleon:
Tested ACK 0fcbfdb7ad. Ran `cargo run` with various of the individual tests and with bad input. Also ran it with no arguments. Everything works as expected and help message looks good.
Tree-SHA512: 48fe4aa9fbb2acef5f8e3c17382ae22e0e350ae6ad9aeeb1a3c0a9192de98809f98728e32b8db24a36906ace999e35626ebd6cb2ca05f74146d21e9b6fb14615
429ec1aaaa refactor: Rename CTransaction::nVersion to version (Ava Chow)
27e70f1f5b consensus: Store transaction nVersion as uint32_t (Ava Chow)
Pull request description:
Given that the use of a transaction's nVersion is always as an unsigned int, it doesn't make sense to store it as signed and then cast it to unsigned everywhere it is used and displayed.
Since a few alternative implementations have recently been revealed to have made an error with this signedness that would have resulted in consensus failure, I think it makes sense for us to just make this always unsigned to make it clear that the version is treated as unsigned. This would also help us avoid future potential issues with signedness of this value.
I believe that this is safe and does not actually change what transactions would or would not be considered both standard and consensus valid. Within consensus, the only use of the version in consensus is in BIP68 validation which was already casting it to uint32_t. Within policy, although it is used as a signed int for the transaction version number check, I do not think that this change would change standardness. Standard transactions are limited to the range [1, 2]. Negative numbers would have fallen under the < 1 condition, but by making it unsigned, they are still non-standard under the > 2 condition.
Unsigned and signed ints are serialized and unserialized the same way so there is no change in serialization.
ACKs for top commit:
maflcko:
ACK 429ec1aaaa 🐿
glozow:
ACK 429ec1aaaa
shaavan:
ACK 429ec1aaaa💯
Tree-SHA512: 0bcd92a245d7d16c3665d2d4e815a4ef28207ad4a1fb46c6f0203cdafeab1b82c4e95e4bdce7805d80a4f4a46074f6542abad708e970550d38a00d759e3dcef1
47f705b33f tests: add fuzz tests for BitSet (Pieter Wuille)
59a6df6bd5 util: add BitSet (Pieter Wuille)
Pull request description:
Extracted from #30126.
This introduces the `BitSet` data structure, inspired by `std::bitset`, but with a few features that cannot be implemented on top without efficiency loss:
* Finding the first set bit (`First`)
* Finding the last set bit (`Last`)
* Iterating over all set bits (`begin` and `end`).
And a few other operators/member functions that help readability for #30126:
* `operator-` for set subtraction
* `Overlaps()` for testing whether intersection is non-empty
* `IsSupersetOf()` for testing (non-strict) supersetness
* `IsSubsetOf()` for testing (non-strict) subsetness
* `Fill()` to construct a set with all numbers from 0 to n-1, inclusive
* `Singleton()` to construct a set with one specific element.
Everything is tested through a simulation-based fuzz test that compares the behavior with normal `std::bitset` equivalent operations.
ACKs for top commit:
instagibbs:
ACK 47f705b33f
achow101:
ACK 47f705b33f
cbergqvist:
re-ACK 47f705b33f
theStack:
Code-review ACK 47f705b33f
Tree-SHA512: e451bf4b801f193239ee434b6b614f5a2ac7bb49c70af5aba24c2ac0c54acbef4672556800e4ac799ae835632bdba716209c5ca8c37433a6883dab4eb7cd67c1
24bc46c83b cli: Add warning for duplicate port definition (tdb3)
e208fb5d3b cli: Sanitize ports in rpcconnect and rpcport (tdb3)
Pull request description:
Adds invalid port detection to bitcoin-cli for -rpcconnect and -rpcport.
In addition to detecting malformed/invalid ports (e.g. those outside of the 16-bit port range, not numbers, etc.), bitcoin-cli also now considers usage of port 0 to be invalid. bitcoin-cli previously considered port 0 to be valid and attempted to use it to reach bitcoind.
Functional tests were added for invalid port detection as well as port prioritization.
Additionally, a warning is provided when a port is specified in both -rpcconnect and -rpcport.
This PR is an alternate approach to PR #27820 (e.g. SplitHostPort is unmodified).
Considered an alternative to 127.0.0.1 being specified in functional tests, but at first glance, this might need an update to test_framework/util.py (e.g. rpc_url), which might be left to a future PR.
ACKs for top commit:
S3RK:
light code review ACK 24bc46c83b
achow101:
ACK 24bc46c83b
cbergqvist:
re ACK 24bc46c83b
Tree-SHA512: c83ab6a30a08dd1ac8b368a7dcc2b4f23170f0b61dd67ffcad7bcda05096d333bcb9821fba11018151f55b2929c0a333bfec15b8bb863d83f41fc1974c6efca5
