ce8094246ee95232e9d84f7e37f3c0a43ef587ce random: replace construct/assign with explicit Reseed() (Pieter Wuille)
2ae392d561ecfdf81855e6df6b9ad3d8843cdfa2 random: use LogError for init failure (Pieter Wuille)
97e16f57042cab07e5e73f6bed19feec2006e4f7 tests: make fuzz tests (mostly) deterministic with fixed seed (Pieter Wuille)
2c91330dd68064e402e8eceea3df9474bb7afd48 random: cleanup order, comments, static (Pieter Wuille)
8e31cf9c9b5e9fdd01e8b220c08a3ccde5cf584c net, net_processing: use existing RNG objects more (Pieter Wuille)
d5fcbe966bc501db8bf6a3809633f0b82e6ae547 random: improve precision of MakeExponentiallyDistributed (Pieter Wuille)
cfb0dfe2cf0b46f3ea9e62992ade989860f086c8 random: convert GetExponentialRand into rand_exp_duration (Pieter Wuille)
4eaa239dc3e189369d59144b524cb2808cbef8c3 random: convert GetRand{Micros,Millis} into randrange (Pieter Wuille)
82de1b80d95fc9447e64c098dcadb6b8a2f1f2ee net: use GetRandMicros for cache expiration (Pieter Wuille)
ddc184d999d7e1a87efaf6bcb222186f0dcd87ec random: get rid of GetRand by inlining (Pieter Wuille)
e2d1f84858485650ff743753ffa5c679f210a992 random: make GetRand() support entire range (incl. max) (Pieter Wuille)
810cdf6b4e12a1fdace7998d75b4daf8b67d7028 tests: overhaul deterministic test randomness (Pieter Wuille)
6cfdc5b104caf9952393f9dac2a36539d964077f random: convert XoRoShiRo128PlusPlus into full RNG (Pieter Wuille)
8cc2f45065fc1864f879248d1e1444588e27076b random: move XoRoShiRo128PlusPlus into random module (Pieter Wuille)
8f5ac0d0b608bdf396d8f2d758a792f869c2cd2a xoroshiro128plusplus: drop comment about nonexisting copy() (Pieter Wuille)
8924f5120f66269c04633167def01f82c74ea730 random: modernize XoRoShiRo128PlusPlus a bit (Pieter Wuille)
ddb7d26cfd96c1f626def4755e0e1b5aaac94d3e random: add RandomMixin::randbits with compile-known bits (Pieter Wuille)
21ce9d8658fed0d3e4552e8b02a6902cb31c572e random: Improve RandomMixin::randbits (Pieter Wuille)
9b14d3d2da05f74ffb6a2ac20b7d9efefbe29634 random: refactor: move rand* utilities to RandomMixin (Pieter Wuille)
40dd86fc3b60d7a67a9720a84a685f16e3f05b06 random: use BasicByte concept in randbytes (Pieter Wuille)
27cefc7fd6a6a159779f572f4c3a06170f955ed8 random: add a few noexcepts to FastRandomContext (Pieter Wuille)
b3b382dde202ad508baf553817c5b38fdd2d4a0c random: move rand256() and randbytes() to .h file (Pieter Wuille)
493a2e024e845e623e202e3eefe1cc2010e9b514 random: write rand256() in function of fillrand() (Pieter Wuille)
Pull request description:
This PR contains a number of vaguely-related improvements to the random module.
The specific changes and more detailed rationale is in the commit messages, but the highlights are:
* `XoRoShiRo128PlusPlus` (previously a test-only RNG) moves to random.h and becomes `InsecureRandomContext`, which is even faster than `FastRandomContext` but non-cryptographic. It also gets all helper randomness functions (`randrange`, `fillrand`, ...), making it a lot more succinct to use.
* During tests, **all** randomness is made deterministic (except for `GetStrongRandBytes`) but non-repeating (like `GetRand()` used to be when `g_mock_deterministic_tests` was used), either fixed, or from a random seed (overridden by env var).
* Several infrequently used top-level functions (`GetRandMillis`, `GetRandMicros`, `GetExponentialRand`) are converted into member functions of `FastRandomContext` (and `InsecureRandomContext`).
* `GetRand<T>()` (without argument) can now return the maximum value of the type (previously e.g. `GetRand<uint32_t>()` would never return 0xffffffff).
ACKs for top commit:
achow101:
ACK ce8094246ee95232e9d84f7e37f3c0a43ef587ce
maflcko:
re-ACK ce8094246ee95232e9d84f7e37f3c0a43ef587ce 🐈
hodlinator:
ACK ce8094246ee95232e9d84f7e37f3c0a43ef587ce
dergoegge:
utACK ce8094246ee95232e9d84f7e37f3c0a43ef587ce
Tree-SHA512: 79bc0cbafaf27e95012c1ce2947a8ca6f9a3c78af5f1f16e69354b6fc9b987a28858adf4cd356dc5baf21163e9af8dcc24e70f8d7173be870e8a3ddcdd47c02c
Updating to MLC v0.18.0 includes a new feature which will ignore all
files ignored by git: `--gitignore`.
This helps avoid false-positives flagged by this linter in non-project
files, such as a developer might expect to have in their directory (e.g.
guix-builds, python venvs, etc.)
2342b46c451658a418f8e28e50b2ad0e5abd284d test: Add coverage for getchaintxstats in assumeutxo context (Fabian Jahr)
faf2a6750b2da97a18c48a3acf9c9da2aebe86d0 rpc: Reorder getchaintxstats output (MarcoFalke)
fa2dada0c9ab61266bcca86fcd28ced873976916 rpc: Avoid getchaintxstats invalid results (MarcoFalke)
Pull request description:
The `getchaintxstats` RPC reply during AU background download may return non-zero, but invalid, values for `window_tx_count` and `txrate`.
For example, `txcount` may be zero for a to-be-downloaded block, but may be non-zero for an ancestor block which is already downloaded. Thus, the values returned may be negative (and cause intermediate integer sanitizer violations).
Also, `txcount` may be accurate for the snapshot base block, or a descendant of it. However it may be zero for an ancestor block that still needs to be downloaded. Thus, the values returned may be positive, but wrong.
Fix all issues by skipping the returned value if either `txcount` is unset (equal to zero).
Also, skip `txcount` in the returned value, if it is unset (equal to zero).
Fixes https://github.com/bitcoin/bitcoin/issues/29328
ACKs for top commit:
fjahr:
re-ACK 2342b46c451658a418f8e28e50b2ad0e5abd284d
achow101:
ACK 2342b46c451658a418f8e28e50b2ad0e5abd284d
mzumsande:
ACK 2342b46c451658a418f8e28e50b2ad0e5abd284d
Tree-SHA512: 931cecc40ee5dc0f96be728db7eb297155f8343076cd29c8b8c050c99fd1d568b80f54c9459a34ca7a9489c2474c729796d00eeb1934d6a9f7b4d6a53e3ec430
926b8e39dcbc0a3a8a75ef0a29bdca2bf738d746 [doc] add release note for TRUC (glozow)
19a9b90617419f68d0f1c90ee115b5220be99a16 use version=3 instead of v3 in debug strings (glozow)
881fac8e609be17eb71bd9a54c0284b304e2e2e2 scripted-diff: change names from V3 to TRUC (glozow)
a573dd261748d2a80560f73db08f7dca788c7fcf [doc] replace mentions of v3 with TRUC (glozow)
089b5757dff39a9a06cdb625aaced9beeb72958d rename mempool_accept_v3.py to mempool_truc.py (glozow)
f543852a89d93441645250c40c3980aeb0c3b664 rename policy/v3_policy.* to policy/truc_policy.* (glozow)
Pull request description:
Adds a release note for TRUC policy which will be live in v28.0.
For clarity, replaces mentions of "v3" with "TRUC" in most places. Suggested in
- https://github.com/bitcoin/bitcoin/pull/29496#discussion_r1629749583
- https://github.com/bitcoin/bitcoin/pull/29496#discussion_r1624500904
I changed error strings from "v3-violation" to "TRUC-violation" but left v3 in the debug strings because I think it might be clearer for somebody who is debugging. Similarly, I left some variables unchanged because I think they're more descriptive this way, e.g. `tx_v3_from_v2_and_v3`. I'm happy to debate places that should or shouldn't be documented differently in this PR, whatever is clearest to everyone.
ACKs for top commit:
instagibbs:
reACK 926b8e39dc
achow101:
ACK 926b8e39dcbc0a3a8a75ef0a29bdca2bf738d746
ismaelsadeeq:
Code review ACK 926b8e39dcbc0a3a8a75ef0a29bdca2bf738d746
Tree-SHA512: 16c88add0a29dc6d1236c4d45f34a17b850f6727b231953cbd52eb9f7268d1d802563eadfc8b7928c94ed3d7a615275dd103e57e81439ebf3ba2b12efa1e42af
2f9bde69f45c7a9fdcf0c65f9e1305391a6f1f28 test: Remove unnecessary restart in assumeutxo test (Fabian Jahr)
19ce3d407ef546fa50d18b2ffbd67b7417797064 assumeutxo: Check snapshot base block is not marked invalid (Fabian Jahr)
80315c011863d69e7785673283e4c9033fbcd5ac refactor: Move early loadtxoutset checks into ActiveSnapshot (Fabian Jahr)
Pull request description:
This was discovered in a discussion in #29996
If the base block of the snapshot is marked invalid or part of an invalid chain, we currently still load the snapshot and get stuck in a weird state where we have the snapshot chainstate but it will never connect to our valid chain.
While this scenario is highly unlikely to occur on mainnet, it still seems good to prevent this inconsistent state.
The behavior change described above is in the second commit.
The first commit refactors the early checks in the `loadtxoutset` RPC by moving them into `ActivateSnapshot()` in order to have the chance to cover them by unit tests in the future and have a more consistent interface. Previously checks were spread out between `rpc/blockchain.cpp` and `validation.cpp`. In order to be able to return the error message to users of the RPC, the return type of `ActivateSnapshot()` is changed from `bool` to `util::Result`.
The third commit removes an unnecessary restart introduced in #29428.
ACKs for top commit:
mzumsande:
re-ACK 2f9bde6
alfonsoromanz:
Re-ACK 2f9bde69f45c7a9fdcf0c65f9e1305391a6f1f28. The RPC code looks much cleaner after the refactor. Also, it seems very useful to get the error message in the RPC response rather than having to rely on the logs in some scenarios if you are an RPC user.
achow101:
ACK 2f9bde69f45c7a9fdcf0c65f9e1305391a6f1f28
Tree-SHA512: 5328dd88c3c7be3f1be97c9eef52ac3666c27188c30a798b3e949f3ffcb83be075127c107e4046f7f39f961a79911ea3d61b61f3c11e451b3e4c541c264eeed4
8ec24bdad89e2a72c394060ba5661a91f374b874 test: Added coverage to Block not found error using gettxoutsetinfo (kevkevinpal)
Pull request description:
#### Description
There were no tests that checked for the `Block not found` error called in `ParseHashOrHeight` when using `gettxoutsetinfo`, this change adds coverage to it.
You can see there are no tests that do the following by doing the below
`grep -nri "Block not found.*gettxoutsetinfo" ./test/functional/`
which leads to no results
ACKs for top commit:
achow101:
ACK 8ec24bdad89e2a72c394060ba5661a91f374b874
tdb3:
ACK 8ec24bdad89e2a72c394060ba5661a91f374b874
kristapsk:
ACK 8ec24bdad89e2a72c394060ba5661a91f374b874
brunoerg:
crACK 8ec24bdad89e2a72c394060ba5661a91f374b874
alfonsoromanz:
Re ACK 8ec24bdad89e2a72c394060ba5661a91f374b874
Tree-SHA512: 2c61c681e7304c679cc3d7dd13af1b795780e85716c25c7423d68104e253d01271e048e21bc21be35dbc7ec1a4fde94e439542f3cfd669fe5a16478c5fa982ab
e38eadb2c2d93d2ee3c9accb649b2de144b3732e test: change comments to `self.log.info` for `test_addnode_getaddednodeinfo` (brunoerg)
c838e3b6106adfe3fe3173aaf5b0a7dee023adce test: add coverage for `node` field of `getaddednodeinfo` RPC (brunoerg)
Pull request description:
We currently do not test a successful call to `getaddednodeinfo` filtering by `node`, we only test it with an unknown address and checks whether it fails. This PR adds coverage to it.
ACKs for top commit:
kevkevinpal:
ACK [e38eadb](e38eadb2c2)
achow101:
ACK e38eadb2c2d93d2ee3c9accb649b2de144b3732e
tdb3:
re ACK e38eadb2c2d93d2ee3c9accb649b2de144b3732e
BrandonOdiwuor:
Code Review ACK e38eadb2c2d93d2ee3c9accb649b2de144b3732e
rkrux:
tACK [e38eadb](e38eadb2c2)
Tree-SHA512: e9f768b7aa86e58b0b0ced089ead57040ff9a5204493da1ab99c8bc897b6dcdce7c856855f74c52010fceef19af1e12a39eee9f8f2e7294b42476b6f980fe754
Keep mentions of v3 in debug strings to help people who might not know
that TRUC is applied when version=3.
Also keep variable names in tests, as it is less verbose to keep v3 and v2.
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
cc0b4c3b62367a2aebe5fc1f4d0ed4b97e9c2ac9).
73f0a6cbd0b628675028fbd5a37eff8115e7ccfe doc: detail -rpccookieperms option (willcl-ark)
d2afa2690cceb0012b2aa1960e1cfa497f3103fa test: add rpccookieperms test (willcl-ark)
f467aede78533dac60a118e1566138d65522c213 init: add option for rpccookie permissions (willcl-ark)
7df03f1a923e239cea8c9b0d603a9eb00863a40c 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 73f0a6cbd0b628675028fbd5a37eff8115e7ccfe
ryanofsky:
Code review ACK 73f0a6cbd0b628675028fbd5a37eff8115e7ccfe. 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 73f0a6cbd0b628675028fbd5a37eff8115e7ccfe
Tree-SHA512: e800d59a44aca10e1c58ca69bf3fdde9f6ccf5eab4b7b962645af6d6bc0cfa3a357701e409c8c60d8d7744fcd33a91e77ada11790aa88cd7811ef60fab86ab11
72b226882fe2348a9a66aee1d8d21b4e2d275e68 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 72b226882fe2348a9a66aee1d8d21b4e2d275e68
tdb3:
re ACK for 72b226882fe2348a9a66aee1d8d21b4e2d275e68
rkrux:
tACK [72b2268](72b226882f)
ismaelsadeeq:
utACK 72b226882fe2348a9a66aee1d8d21b4e2d275e68
Tree-SHA512: d77be19231023383a9c79a5d66b642dcbc6ebfc31a363e0b9f063c44898720a7859ec211cdbc0914ac7a3bfdf15e52fb8fc20d97f171431f70492c0f159dbc36
a9716c53f05082d6d89ebea51a46d4404efb12d7 rpc: call IsInitialBlockDownload via miner interface (Sjors Provoost)
dda0b0834faf7be7e8938bf63e7bb01cd54a416a rpc: minize getTipHash() calls in gbt (Sjors Provoost)
7b4d3249ced93ec5986500e43b324005ed89502f rpc: call processNewBlock via miner interface (Sjors Provoost)
9e228351e761d8d24413bbc4ac1610b4f3dec2bf rpc: getTransactionsUpdated via miner interface (Sjors Provoost)
64ebb0f97178687517c2060bf6b9931064607888 Always pass options to BlockAssembler constructor (Sjors Provoost)
4bf2e361da1964f7c278b4939967a0e5afde20b0 rpc: call CreateNewBlock via miner interface (Sjors Provoost)
404b01c436122b951e9e06ed26d79dba4651685e rpc: getblocktemplate getTipHash() via Miner interface (Sjors Provoost)
d8a3496b5ad27bea4c79ea0344f595cc1b95f0d3 rpc: call TestBlockValidity via miner interface (Sjors Provoost)
8ecb6816781c7c7f423b501cbb2de3abd7250119 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 a9716c53f05082d6d89ebea51a46d4404efb12d7
itornaza:
Code review and std-tests ACK a9716c53f05082d6d89ebea51a46d4404efb12d7
ryanofsky:
Code review ACK a9716c53f05082d6d89ebea51a46d4404efb12d7 with one minor suggestion in case you update. Only changes since last review were other small changes to the interface.
Tree-SHA512: cf97f87d6e9ed89da3835a0730da3b24a7b14c8605ea221149103a5915e79598cf082a95f2bc88e33f1c450e3d4aad88aed1163a29195acca88bcace055af724
6eecba475efd025eb011400af58621ad5823994e net_processing: make MaybePunishNodeFor{Block,Tx} return void (Pieter Wuille)
ae60d485da33f238ed2186799da4e109d4edd3a1 net_processing: remove Misbehavior score and increments (Pieter Wuille)
6457c311977bba3585648e32e3bd5754028aa292 net_processing: make all Misbehaving increments = 100 (Pieter Wuille)
5120ab1478c200b18ee621a6ffa0362f4e991959 net_processing: drop 8 headers threshold for incoming BIP130 (Pieter Wuille)
944c54290d5c081dc433dae7e7941074a3a8b5a7 net_processing: drop Misbehavior for unconnecting headers (Pieter Wuille)
9f66ac7cf1931c4d7c36abbb000b7de306d83a4c 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 6eecba475efd025eb011400af58621ad5823994e
mzumsande:
Code Review ACK 6eecba475efd025eb011400af58621ad5823994e
glozow:
light code review / concept ACK 6eecba475efd025eb011400af58621ad5823994e
Tree-SHA512: e11e8a652c4ec048d8961086110a3594feefbb821e13f45c14ef81016377be0db44b5311751ef635d6e026def1960aff33f644e78ece11cfb54f2b7daa96f946
94ed4fbf8e1a396c650b5134d396d6c0be35ce10 Add release note for size 2 package rbf (Greg Sanders)
afd52d8e63ed323a159ea49fd1f10542abeacb97 doc: update package RBF comment (Greg Sanders)
6e3c4394cfadf32c06c8c4732d136ca10c316721 mempool: Improve logging of replaced transactions (Greg Sanders)
d3466e4cc5051c314873dd14ec8f7a88494c0780 CheckPackageMempoolAcceptResult: Check package rbf invariants (Greg Sanders)
316d7b63c97144ba3e21201315c784852210f8ff Fuzz: pass mempool to CheckPackageMempoolAcceptResult (Greg Sanders)
4d15bcf448eb3c4451b63e8f78cc61f3f9f9b639 [test] package rbf (glozow)
dc21f61c72e5a97d974ca2c5cb70b8328f4fab2a [policy] package rbf (Suhas Daftuar)
5da396781589177d4ceb3b4b59c9f309a5e4d029 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 94ed4fbf8e1a396c650b5134d396d6c0be35ce10
glozow:
reACK 94ed4fbf8e via range-diff
ismaelsadeeq:
re-ACK 94ed4fbf8e1a396c650b5134d396d6c0be35ce10
theStack:
Code-review ACK 94ed4fbf8e1a396c650b5134d396d6c0be35ce10
murchandamus:
utACK 94ed4fbf8e1a396c650b5134d396d6c0be35ce10
Tree-SHA512: 9bd383e695964f362f147482bbf73b1e77c4d792bda2e91d7f30d74b3540a09146a5528baf86854a113005581e8c75f04737302517b7d5124296bd7a151e3992
881724d443d11f984a721ef1edd5777c24d1ed29 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 881724d443d11f984a721ef1edd5777c24d1ed29
tdb3:
re ACK for 881724d443d11f984a721ef1edd5777c24d1ed29
rkrux:
tACK [881724](881724d443)
Tree-SHA512: c5dff20cf014d0181f49d6b161f1364e1c6b79e8661047f77f07e21e59f4d1f2fd6f745538c8fc5bd6d4244650a840dd64d184634366f7c21fa67141a60af44a
5cf0a1f230389ef37e0ff65de5fc98394f32f60c test: add `createmultisig` P2MS encoding test for all n (1..20) (Sebastian Falbesoner)
0570d2c204ec7f10af6bd8e48c23318a48fefc10 test: add unit test for `keys_to_multisig_script` (Sebastian Falbesoner)
0c41fc3fa52ad16923afbd0ec18b9c1b3ded8036 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 5cf0a1f230389ef37e0ff65de5fc98394f32f60c
tdb3:
ACK 5cf0a1f230389ef37e0ff65de5fc98394f32f60c
rkrux:
reACK [5cf0a1f](5cf0a1f230)
Tree-SHA512: 4168a165c3f483ec8e37a27dba1628a7ea0063545a2b7e74d9e20d753fddd7e33d37e1a190434fa6dca39adf9eef5d0211f7a0c1c7b44979f0a3bb350e267562
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
f58beabe754363cb7d5b24032fd392654b9514ac test: bumpfee with user specified fee_rate ignores walletIncrementalRelayFee (ismaelsadeeq)
436e88f4336199998184cbfa5d1c889ffaefbfb5 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 f58beabe754363cb7d5b24032fd392654b9514ac
murchandamus:
ACK f58beabe754363cb7d5b24032fd392654b9514ac
Tree-SHA512: 193259f87173b7d5a8e68e0e29f2ca7e75c550e3cf0dee3d6d822b5b1e07c2e6dec0bfc8fb435855736ebced97a10dbdbfef72e8c5abde06fdefcba122f2e7f1
e2779ce98b39e14cada08a654928e798436f5a46 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 e2779ce98b39e14cada08a654928e798436f5a46
kevkevinpal:
ACK [e2779ce](e2779ce98b)
tdb3:
ACK e2779ce98b39e14cada08a654928e798436f5a46
BrandonOdiwuor:
Code Review ACK e2779ce98b39e14cada08a654928e798436f5a46
Tree-SHA512: 41c7e990684b60645cf4ccec8aad5ebbe61da221871eb3c1685b2bb1eebda58b29358502cb1525b7c7a2b612e2bebf449ed0bae14ab663b4641c528a9c013b5b
0fcbfdb7ad172e518a10dd6e5be4cb6bb1158784 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 0fcbfdb7ad172e518a10dd6e5be4cb6bb1158784
achow101:
ACK 0fcbfdb7ad172e518a10dd6e5be4cb6bb1158784
marcofleon:
Tested ACK 0fcbfdb7ad172e518a10dd6e5be4cb6bb1158784. 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
429ec1aaaaafab150f11e27fcf132a99b57c4fc7 refactor: Rename CTransaction::nVersion to version (Ava Chow)
27e70f1f5be1f536f2314cd2ea42b4f80d927fbd 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 429ec1aaaaafab150f11e27fcf132a99b57c4fc7 🐿
glozow:
ACK 429ec1aaaa
shaavan:
ACK 429ec1aaaaafab150f11e27fcf132a99b57c4fc7 💯
Tree-SHA512: 0bcd92a245d7d16c3665d2d4e815a4ef28207ad4a1fb46c6f0203cdafeab1b82c4e95e4bdce7805d80a4f4a46074f6542abad708e970550d38a00d759e3dcef1
