Age | Commit message (Collapse) | Author |
|
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
|
|
node::Warnings and remove globals
260f8da71a35232d859d7705861fc1a88bfbbe81 refactor: remove warnings globals (stickies-v)
9c4b0b7ce459765fa1a63b410c3423b90f0d2a5f node: update uiInterface whenever warnings updated (stickies-v)
b071ad9770b7ae7fc718dcbfdc8f62dffbf6cfee introduce and use the generalized `node::Warnings` interface (stickies-v)
20e616f86444d00712ac7eb840666e2b0378af4a move-only: move warnings from common to node (stickies-v)
bed29c481aebeb2b0160450c63c03cc68fb89bc6 refactor: remove unnecessary AppendWarning helper function (stickies-v)
Pull request description:
This PR:
- moves warnings from common to the node library and into the node namespace (as suggested in https://github.com/bitcoin/bitcoin/pull/29845#discussion_r1570069541)
- generalizes the warnings interface to `Warnings::Set()` and `Warnings::Unset()` methods, instead of having a separate function and globals for each warning. As a result, this simplifies the `kernel::Notifications` interface.
- removes warnings.cpp from the kernel library
- removes warning globals
- adds testing for the warning logic
Behaviour change introduced:
- the `-alertnotify` command is executed for all `KernelNotifications::warningSet` calls, which now also covers the `LARGE_WORK_INVALID_CHAIN` warning
- the GUI is updated automatically whenever a warning is (un)set, covering some code paths where it previously wouldn't be, e.g. when `node::AbortNode()` is called, or for the `LARGE_WORK_INVALID_CHAIN` warning
Some discussion points:
- ~is `const std::string& id` the best way to refer to warnings? Enums are an obvious alternative, but since we need to define warnings across libraries, strings seem like a straightforward solution.~ _edit: updated approach to use `node::Warning` and `kernel::Warning` enums._
ACKs for top commit:
achow101:
ACK 260f8da71a35232d859d7705861fc1a88bfbbe81
ryanofsky:
Code review ACK 260f8da71a35232d859d7705861fc1a88bfbbe81. Only change since last review was rebasing
TheCharlatan:
Re-ACK 260f8da71a35232d859d7705861fc1a88bfbbe81
Tree-SHA512: a3fcedaee0d3ad64e9c111aeb30665162f98e0e72acd6a70b76ff2ddf4f0a34da4f97ce353c322a1668ca6ee4d8a81cc6e6d170c5bbeb7a43cffdaf66646b588
|
|
when user specifies fee_rate
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
|
|
fae3a1f0065064d80ab4c0375a9eaeb666c5dd55 log: use error level for critical log messages (MarcoFalke)
Pull request description:
This picks up the first commit from https://github.com/bitcoin/bitcoin/pull/29231, but extends it to also cover cases that were missed in it.
As per https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#logging, LogError should be used for severe problems that require the node to shut down.
ACKs for top commit:
stickies-v:
re-ACK fae3a1f0065064d80ab4c0375a9eaeb666c5dd55, I'm ~0 on the latest force push as `user_error` was already logged at the right level through `GetNotifications().fatalError(user_error);` so I'd be in favour of deduplicating/cleaning up this logging logic but can be done in follow-up.
kevkevinpal:
ACK [fae3a1f](https://github.com/bitcoin/bitcoin/pull/30255/commits/fae3a1f0065064d80ab4c0375a9eaeb666c5dd55)
achow101:
ACK fae3a1f0065064d80ab4c0375a9eaeb666c5dd55
Tree-SHA512: 3f99fd25d5a204d570a42d8fb2b450439aad7685692f9594cc813d97253c4df172a6ff3cf818959bfcf25dfcf8ee9a9c9ccc6028fcfcecdb47591e18c77ef246
|
|
a37778d4d32b4ddeff96f68a130dc8da3a84b278 Squashed 'src/leveldb/' changes from e2f10b4e47..688561cba8 (fanquake)
Pull request description:
Includes https://github.com/bitcoin-core/leveldb-subtree/pull/41 which is used in #30234.
ACKs for top commit:
theuni:
utACK 95812d912b6335caa7af2a084d84447fb4aad156
Tree-SHA512: 3d943695a3d33816cf5558b183f5629aa92a500a1544eecedf84952e93c8592a8cf0d554b88281fc0bad3c9e920ebcff1ed8edc12f8e73f36ed5335482beb829
|
|
bech32 encoding
07f64177a49f1b6b4d486d10cf67fddfa3c995eb Reduce memory copying operations in bech32 encode (Lőrinc)
d5ece3c4b5e109f65f5d3315c43239dd87bb2c81 Reserve hrp memory in Decode and LocateErrors (Lőrinc)
Pull request description:
Started optimizing the base conversions in [TryParseHex](https://github.com/bitcoin/bitcoin/pull/29458), [Base58](https://github.com/bitcoin/bitcoin/pull/29473) and [IsSpace](https://github.com/bitcoin/bitcoin/pull/29602) - this is the next step.
Part of this change was already merged in https://github.com/bitcoin/bitcoin/pull/30047, which made decoding `~26%` faster.
Here I've reduced the memory reallocations and copying operations in bech32 encode, making it `~15%` faster.
> make && ./src/bench/bench_bitcoin --filter='Bech32Encode' --min-time=1000
Before:
```
| ns/byte | byte/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 19.97 | 50,074,562.72 | 0.1% | 1.06 | `Bech32Encode`
```
After:
```
| ns/byte | byte/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 17.33 | 57,687,668.20 | 0.1% | 1.10 | `Bech32Encode`
```
ACKs for top commit:
josibake:
ACK https://github.com/bitcoin/bitcoin/pull/29607/commits/07f64177a49f1b6b4d486d10cf67fddfa3c995eb
sipa:
utACK 07f64177a49f1b6b4d486d10cf67fddfa3c995eb
achow101:
ACK 07f64177a49f1b6b4d486d10cf67fddfa3c995eb
Tree-SHA512: 511885217d044ad7ef2bdf9203b8e0b94eec8b279bc193bb7e63e29ab868df6d21e9e4c7a24390358e1f9c131447ee42039df72edcf1e2b11e1856eb2b3e10dd
|
|
|
|
|
|
|
|
|
|
Support package RBF where the conflicting package would result
in a mempool cluster of size two, and each of its direct
conflicts are also part of an up-to-size-2 mempool cluster.
This restricted topology allows for exact calculation
of miner scores for each side of the equation, reducing
the surface area for new pins, or incentive-incompatible
replacements.
This allows wallets to create simple CPFP packages
that can fee bump other simple CPFP packages. This,
leveraged with other restrictions such as V3 transactions,
can create pin-resistant applications.
Future package RBF relaxations can be considered when appropriate.
Co-authored-by: glozow <gloriajzhao@gmail.com>
Co-authored-by: Greg Sanders <gsanders87@gmail.com>
|
|
|
|
eb37a9b8e79f9e49d73b96a49bf97a96d9eb676c
cb59af38e72ab189b052ec84e2d1027473235ba4 Squashed 'src/minisketch/' changes from 3472e2f5ec..eb37a9b8e7 (fanquake)
Pull request description:
Includes https://github.com/sipa/minisketch/pull/87 which is used in https://github.com/bitcoin/bitcoin/pull/30234.
Includes https://github.com/sipa/minisketch/pull/88 which is used in https://github.com/bitcoin/bitcoin/pull/29876.
ACKs for top commit:
sipa:
utACK 89464ad59cf11f68315ea3104236989e5b429d15
theuni:
utACK 89464ad59cf11f68315ea3104236989e5b429d15
Tree-SHA512: 838a8c60856bfdf714da7d5d97e31d458290849ba5007d5c5bb7abb83d413ada6b4c16e45b0e060ff892b5785e6b664be9b6a666d04f0a414b0e359d64d3ad44
|
|
|
|
This commit introduces slight behaviour change. Previously, the
GUI status bar would be updated for most warnings, namely
UNKNOWN_NEW_RULES_ACTIVATED, CLOCK_OUT_OF_SYNC and
PRE_RELEASE_TEST_BUILD, but not for LARGE_WORK_INVALID_CHAIN
(and not for FATAL_INTERNAL_ERROR, but that is not really
meaningful).
Fix this by always updating the status bar when the warnings are
changed.
|
|
Instead of having separate warning functions (and globals) for each
different warning that can be raised, encapsulate this logic into
a single class and allow to (un)set any number of warnings.
Introduces behaviour change:
- the `-alertnotify` command is executed for all
`KernelNotifications::warningSet` calls, which now also covers the
`LARGE_WORK_INVALID_CHAIN` warning.
- previously, warnings were returned based on a predetermined order,
e.g. with the "pre-release test build" warning always first. This
is no longer the case, and Warnings::GetMessages() will return
messages sorted by the id of the warning.
Removes warnings.cpp from kernel.
|
|
Since rpc/util.cpp is in common, also move GetNodeWarnings() to
node::GetWarningsForRPC()
|
|
|
|
c7376babd19d0c858fef93ebd58338abd530c1f4 doc: Clarify distinction between util and common libraries in libraries.md (Ryan Ofsky)
4f74c59334d496f28e1a5c0d84c412f9020b366f util: Move util/string.h functions to util namespace (Ryan Ofsky)
4d05d3f3b42a41525aa6ec44b90f543dfab53ecf util: add TransactionError includes and namespace declarations (Ryan Ofsky)
680eafdc74021c1e0893c3a62404e607fd4724f5 util: move fees.h and error.h to common/messages.h (Ryan Ofsky)
02e62c6c9af4beabaeea58fb1ea3ad0dc5094678 common: Add PSBTError enum (Ryan Ofsky)
0d44c44ae33434f366229c612d6edeedf7658963 util: move error.h TransactionError enum to node/types.h (Ryan Ofsky)
9bcce2608dd2515dc35a0f0866abc9d43903c795 util: move spanparsing.h to script/parsing.h (Ryan Ofsky)
6dd2ad47922694d2ab84bad4dac9dd442c5df617 util: move spanparsing.h Split functions to string.h (Ryan Ofsky)
23cc8ddff472d259605d7790ba98a1900e77efab util: move HexStr and HexDigit from util to crypto (TheCharlatan)
6861f954f8ff42c87ad638037adae86a5bd89600 util: move util/message to common/signmessage (Ryan Ofsky)
cc5f29fbea15d33e4d1aa95591253c6b86953fe7 build: move memory_cleanse from util to crypto (Ryan Ofsky)
5b9309420cc9721a0d5745b6ad3166a4bdbd1508 build: move chainparamsbase from util to common (Ryan Ofsky)
ffa27af24da81a97d6c4912ae0e10bc5b6f17f69 test: Add check-deps.sh script to check for unexpected library dependencies (Ryan Ofsky)
Pull request description:
Remove `fees.h`, `errors.h`, and `spanparsing.h` from the util library. Specifically:
- Move `Split` functions from `util/spanparsing.h` to `util/string.h`, using `util` namespace for clarity.
- Move remaining spanparsing functions to `script/parsing.h` since they are used for descriptor and miniscript parsing.
- Combine `util/fees.h` and `util/errors.h` into `common/messages.h` so there is a place for simple functions that generate user messages to live, and these functions are not part of the util library.
Motivation for this change is that the util library is a dependency of the kernel, and we should remove functionality from util that shouldn't be called by kernel code or kernel applications. These changes should also improve code organization and make functions easier to discover. Some of these same moves are (or were) part of #28690, but did not help with code organization, or made it worse, so it is better to move them and clean them up in the same PR so code only has to change one time.
ACKs for top commit:
achow101:
ACK c7376babd19d0c858fef93ebd58338abd530c1f4
TheCharlatan:
Re-ACK c7376babd19d0c858fef93ebd58338abd530c1f4
hebasto:
re-ACK c7376babd19d0c858fef93ebd58338abd530c1f4.
Tree-SHA512: 5bcef16c1255463b1b69270548711e7ff78ca0dd34e300b95e3ca1ce52ceb34f83d9ddb2839e83800ba36b200de30396e504bbb04fa02c6d0c24a16d06ae523d
|
|
faa41e29d5b90e62179d651f4010272dae685621 fuzz: Use std::span in FuzzBufferType (MarcoFalke)
Pull request description:
The use of `Span` is problematic, because it lacks methods such as `rbegin`, leading to compile failures when used:
```
error: no member named 'rbegin' in 'Span<const unsigned char>'
```
One could fix `Span`, but it seems better to use `std::span`, given that `Span` will be removed anyway in the long term.
ACKs for top commit:
dergoegge:
utACK faa41e29d5b90e62179d651f4010272dae685621
Tree-SHA512: 54bcaf51c83a1b48739cd7f1e8445c6eba0eb04231bce5c35591a47dddb3890ffcaf562cf932930443c80ab0e66950c4619560e6692240de0c52aeef3214facd
|
|
193c748e44f8647a056121fc9cbb9c2efbcbfc49 fuzz: add I2P harness (marcofleon)
Pull request description:
Addresses https://github.com/bitcoin/bitcoin/issues/28803. This updated harness sets mock time at the beginning of each iteration and deletes the private key file at the end of each iteration. Mock time is used to make the fuzz test more stable, as `GetTime` is called at points in `i2p`. Deleting the private key file ensures that each iteration is independent from the last. Now, a new key is generated in `i2p` every time, so the fuzzer can eventually make progress through the target code.
Re-working this harness also led me and dergoegge to resolve a couple of issues in `FuzzedSock`, which allows for full coverage of the `i2p` code. Those changes can be seen in https://github.com/bitcoin/bitcoin/pull/30211.
The SAM protocol for interacting with I2P requires some specifc inputs so it's best to use a dictionary when running this harness.
<details>
<summary>I2P dict</summary>
```
"HELLO VERSION"
"HELLO REPLY RESULT=OK VERSION="
"HELLO REPLY RESULT=NOVERSION"
"HELLO REPLY RESULT=I2P_ERROR"
"SESSION CREATE"
"SESSION STATUS RESULT=OK DESTINATION="
"SESSION STATUS RESULT=DUPLICATED_ID"
"SESSION STATUS RESULT=DUPLICATED_DEST"
"SESSION STATUS RESULT=INVALID_ID"
"SESSION STATUS RESULT=INVALID_KEY"
"SESSION STATUS RESULT=I2P_ERROR MESSAGE="
"SESSION ADD"
"SESSION REMOVE"
"STREAM CONNECT"
"STREAM STATUS RESULT=OK"
"STREAM STATUS RESULT=INVALID_ID"
"STREAM STATUS RESULT=INVALID_KEY"
"STREAM STATUS RESULT=CANT_REACH_PEER"
"STREAM STATUS RESULT=I2P_ERROR MESSAGE="
"STREAM ACCEPT"
"STREAM FORWARD"
"DATAGRAM SEND"
"RAW SEND"
"DEST GENERATE"
"DEST REPLY PUB= PRIV="
"DEST REPLY RESULT=I2P_ERROR"
"NAMING LOOKUP"
"NAMING REPLY RESULT=OK NAME= VALUE="
"DATAGRAM RECEIVED DESTINATION= SIZE="
"RAW RECEIVED SIZE="
"NAMING REPLY RESULT=INVALID_KEY NAME="
"NAMING REPLY RESULT=KEY_NOT_FOUND NAME="
"MIN"
"MAX"
"STYLE"
"ID"
"SILENT"
"DESTINATION"
"NAME"
"SIGNATURE_TYPE"
"CRYPTO_TYPE"
"SIZE"
"HOST"
"PORT"
"FROM_PORT"
"TRANSIENT"
"STREAM"
"DATAGRAM"
"RAW"
"MASTER"
"true"
"false"
```
</details>
I'll add this dict to qa-assets later on.
ACKs for top commit:
dergoegge:
tACK 193c748e44f8647a056121fc9cbb9c2efbcbfc49
brunoerg:
ACK 193c748e44f8647a056121fc9cbb9c2efbcbfc49
vasild:
ACK 193c748e44f8647a056121fc9cbb9c2efbcbfc49
Tree-SHA512: 09ae4b3fa0738aa6f159f4d920493bdbce786b489bc8148e7a135a881e9dba93d727b40f5400c9510e218dd2cfdccc7ce2d3ac9450654fb29c78aac59af92ec3
|
|
|
|
eb37a9b8e7 Merge sipa/minisketch#87: Avoid copy in self-assign
fe6557642e Merge sipa/minisketch#88: build: Add `-Wundef`
8ea298bfa7 Avoid copy in self-assign
978a3d8869 build: Add `-Wundef`
3387044179 Merge sipa/minisketch#86: doc: fix typo in sketch_impl.h
15c2d13b60 doc: fix typo in sketch_impl.h
7be08b8a46 Merge sipa/minisketch#85: Fixes for integer precision loss
00fb4a4d83 Avoid or make integer precision conversion explicit
9d62a4d27c Avoid the need to cast/convert to size_t for vector operations
19e06cc7af Prevent overflows from large capacity/max_elements
git-subtree-dir: src/minisketch
git-subtree-split: eb37a9b8e79f9e49d73b96a49bf97a96d9eb676c
|
|
|
|
f51da34ec1a806d321a468691fa66082eef10ad9 utils: add missing include (Cory Fields)
Pull request description:
Noticed when testing `VecDeque` with no other includes.
For libc++, need type_traits for `std::is_trivially_destructible_v`.
ACKs for top commit:
maflcko:
ACK f51da34ec1a806d321a468691fa66082eef10ad9
glozow:
ACK f51da34ec1a806d321a468691fa66082eef10ad9
sipa:
utACK f51da34ec1a806d321a468691fa66082eef10ad9
Tree-SHA512: bf96910abe9aaddd8586e6cc8f68a9bbac4c26d976ebeebcfa86b86c0da5783c1cbdbc7fa09b62cdcfde19e6442eb65a66bf1e2e80408d68e9dd9689dc22b0fa
|
|
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
|
|
47f705b33fc1381d96c99038e2110e6fe2b2f883 tests: add fuzz tests for BitSet (Pieter Wuille)
59a6df6bd584701f820ad60a10d9d477bf0236b5 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 https://github.com/bitcoin/bitcoin/pull/30160/commits/47f705b33fc1381d96c99038e2110e6fe2b2f883
achow101:
ACK 47f705b33fc1381d96c99038e2110e6fe2b2f883
cbergqvist:
re-ACK 47f705b33fc1381d96c99038e2110e6fe2b2f883
theStack:
Code-review ACK 47f705b33fc1381d96c99038e2110e6fe2b2f883
Tree-SHA512: e451bf4b801f193239ee434b6b614f5a2ac7bb49c70af5aba24c2ac0c54acbef4672556800e4ac799ae835632bdba716209c5ca8c37433a6883dab4eb7cd67c1
|
|
5bc2077e8f592442b089affdf0b5795fbc053bb8 validation: allow to specify frequency for -checkblockindex (Martin Zumsande)
d5a631b9597e5029a5048d9b8ad84ea4536bbac0 validation: improve performance of CheckBlockIndex (Martin Zumsande)
32c80413fdb063199f3bee719c4651bd63f05fce bench: add benchmark for checkblockindex (Martin Zumsande)
Pull request description:
`CheckBlockIndex() ` are consistency checks that are currently enabled by default on regtest.
The function is rather slow, which is annoying if you
* attempt to run it on other networks, especially if not fully synced
* want to generate a long chain on regtest and see block generation slow down because you forgot to disable `-checkblockindex` or don't know it existed.
One reason why it's slow is that in order to be able to traverse the block tree depth-first from genesis, it inserts pointers to all block indices into a `std::multimap` - for which inserts and lookups become slow once there are hundred thousands of entries.
However, typically the block index is mostly chain-like with just a few forks so a multimap isn't really needed for the most part. This PR suggests to store the block indices of the chain ending in the best header in a vector instead, and store only the rest of the indices in a multimap. This does not change the actual consistency checks that are being performed for each index, just the way the block index tree is stored and traversed.
This adds a bit of complication to make sure each block is visited (note that there are asserts that check it), making sure that the two containers are traversed correctly, but it speeds up the function considerably:
On master, a single invocation of `CheckBlockIndex` takes ~1.4s on mainnet for me (4.9s on testnet which has >2.4 million blocks).
With this branch, the runtime goes down to ~0.27s (0.85s on testnet).This is a speedup by a factor ~5.
ACKs for top commit:
achow101:
ACK 5bc2077e8f592442b089affdf0b5795fbc053bb8
furszy:
ACK 5bc2077e8f592442b089affdf0b5795fbc053bb8
ryanofsky:
Code review ACK 5bc2077e8f592442b089affdf0b5795fbc053bb8. Just added suggested assert and simplification since last review
Tree-SHA512: 6b9c3e3e5069d6152b45a09040f962380d114851ff0f9ff1771cf8cad7bb4fa0ba25cd787ceaa3dfa5241fb249748e2ee6987af0ccb24b786a5301b2836f8487
|
|
24bc46c83b39149f4845a575a82337eb46d91bdb cli: Add warning for duplicate port definition (tdb3)
e208fb5d3bea4c1fb750cb0028819635ecdeb415 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 24bc46c83b39149f4845a575a82337eb46d91bdb
achow101:
ACK 24bc46c83b39149f4845a575a82337eb46d91bdb
cbergqvist:
re ACK 24bc46c83b39149f4845a575a82337eb46d91bdb
Tree-SHA512: c83ab6a30a08dd1ac8b368a7dcc2b4f23170f0b61dd67ffcad7bcda05096d333bcb9821fba11018151f55b2929c0a333bfec15b8bb863d83f41fc1974c6efca5
|
|
09ef322acc0a88a9e119f74923399598984c68f6 [[refactor]] Check CTxMemPool options in constructor (TheCharlatan)
Pull request description:
The tests should run the same checks on the mempool options that the init code also applies. The downside to this patch is that the log line may now be printed more than once in the for loop.
This was originally noticed here https://github.com/bitcoin/bitcoin/pull/25290#discussion_r900272797.
ACKs for top commit:
stickies-v:
re-ACK 09ef322acc0a88a9e119f74923399598984c68f6 . Fixed unreachable assert and updated docstring, and also added an exception for "-maxmempool must be at least " in the `tx_pool` fuzz test, which makes sense when looking at how the mempool options are constructed in `SetMempoolConstraints`.
achow101:
ACK 09ef322acc0a88a9e119f74923399598984c68f6
ryanofsky:
Code review ACK 09ef322acc0a88a9e119f74923399598984c68f6. Just fuzz test error checking fix and updated comment since last review
Tree-SHA512: eb3361411c2db70be17f912e3b14d9cb9c60fb0697a1eded952c3b7e8675b7d783780d45c52e091931d1d80fe0f0280cee98dd57a3100def13af20259d9d1b9e
|
|
Noticed when testing VecDeque with no other includes.
For libc++, need type_traits for std::is_trivially_destructible_v.
|
|
test (s/vbytes/weight units)
d1581c6048478cf70c5fb9ec5ebc178f16b376b8 test: doc: fix units in tx size standardness test (s/vbytes/weight units) (Sebastian Falbesoner)
Pull request description:
This small fixup PR is a late follow-up for #17947 (commit 4537ba5f21ad8afb705325cd8e15dd43877eb28f), where the wrong units has been used in the comments for the large tx composition.
ACKs for top commit:
tdb3:
ACK d1581c6048478cf70c5fb9ec5ebc178f16b376b8
ismaelsadeeq:
ACK d1581c6048478cf70c5fb9ec5ebc178f16b376b8
glozow:
ACK d1581c6048478cf70c5fb9ec5ebc178f16b376b8
Tree-SHA512: ea2de42174f9dca0608275ea377c852ebddc5a04a2b32248ce808aea33d7e00cdee3a225b24c0cf426c69646cccbbc31273c62f7bc1647bb3443a61de3b15670
|
|
Relax assumptions about in-mempool children of in-mempool
parents. With package RBF, we will allow a package of size
2 with conflicts on its parent and reconsider the parent
if its fee is insufficient on its own.
Consider:
TxA (in mempool) <- TxB (in mempool)
TxA (in mempool) <- TxB' (in package, conflicts with TxB) <-
TxC (in package)
If TxB' fails to RBF TxB due to insufficient feerate, the
package TxB' + TxC will be considered. PackageV3Checks
called on TxB' will see an in-mempool parent TxA, and
see the in-mempool child TxB. We cannot assume there is
no in-mempool sibling, rather detect it and fail normally.
Prior to package RBF, this would have failed on the first
conflict in package.
|
|
continuing a prior reindex
f68cba29b3be0dec7877022b18a193a3b78c1099 blockman: Replace m_reindexing with m_blockfiles_indexed (Ryan Ofsky)
1b1c6dcca0cc891bd35d29b61628c39098cd94ce test: Add functional test for continuing a reindex (TheCharlatan)
201c1a92824c71ae646d5bba9963871b1d704cc1 indexes: Don't wipe indexes again when already reindexing (TheCharlatan)
804f09dfa116300914e2aeef05ed9710dd504e8c kernel: Add less confusing reindex options (Ryan Ofsky)
e17255322378076edce3ef6f06cd36ca58d2e236 validation: Remove needs_init from LoadBlockIndex (TheCharlatan)
533eab7d67d78f217f74909662133086b79ea808 bugfix: Streamline setting reindex option (TheCharlatan)
Pull request description:
When restarting `bitcoind` during an ongoing reindex without setting the `-reindex` flag again, the block and coins db is left intact, but any data from the optional indexes is discarded. While not a bug per se, wiping the data again is
wasteful, both in terms of having to write it again, as well as potentially leading to longer startup times. So keep the index data instead when continuing a prior reindex.
Also includes a bugfix and smaller code cleanups around the reindexing code. The bug was introduced in b47bd959207e82555f07e028cc2246943d32d4c3: "kernel: De-globalize fReindex".
ACKs for top commit:
stickies-v:
ACK f68cba29b3be0dec7877022b18a193a3b78c1099
fjahr:
Code review ACK f68cba29b3be0dec7877022b18a193a3b78c1099
furszy:
Code review ACK f68cba29b3be0dec7877022b18a193a3b78c1099
ryanofsky:
Code review ACK f68cba29b3be0dec7877022b18a193a3b78c1099. Only changes since last review were cherry-picking suggested commits that rename variables, improving comments, and making some tweaks to test code.
Tree-SHA512: b252228cc76e9f1eaac56d5bd9e4eac23408e0fc04aeffd97a85417f046229364673ee1ca7410b9b6e7b692b03f13ece17c42a10176da0d7e975a8915deb98ca
|
|
|
|
This adds a bitset module that implements a BitSet<N> class, a variant
of std::bitset with a few additional features that cannot be implemented
in a wrapper without performance loss (specifically, finding first and
last bit set, or iterating over all set bits).
|
|
As per doc/developer-notes#logging, LogError should be used for
severe problems that require the node to shut down.
Co-Authored-By: stickies-v <stickies-v@protonmail.com>
|
|
fa780e1c25e8e98253e32d93db65f78a0092433f build: Remove --enable-gprof (MarcoFalke)
Pull request description:
It is unclear what benefit this option has, given that:
* `gprof` requires re-compilation (`perf` and other tools can instead be used on existing executables)
* `gprof` requires hardening to be disabled
* `gprof` doesn't work with `clang`
* `perf` is documented in the dev-notes, and test notes, and embedded into the functional test framework; `gprof` isn't
* Anyone really wanting to use it could pass the required flags to `./configure`
* I couldn't find any mention of the use of `gprof` in the discussions in this repo, apart from the initial pull request adding it (cfaac2a60f3ac63ae8deccb03d88bd559449b78c)
* Keeping it means that it needs to be maintained and ported to CMake
Fix all issues by removing it.
ACKs for top commit:
TheCharlatan:
ACK fa780e1c25e8e98253e32d93db65f78a0092433f
hebasto:
ACK fa780e1c25e8e98253e32d93db65f78a0092433f, I have reviewed the code and it looks OK.
willcl-ark:
crACK fa780e1c25e8e98253e32d93db65f78a0092433f
Tree-SHA512: 0a9ff363ac2bec8b743878a4e3147f18bc16823d00c5007568432c36320bd0199b13b6d0ce828a9a83c2cc434c058afaa64eb2eccfbd93ed85b81ce10c41760c
|
|
15796d4b61342f75548b20a18c670ed21d102ba8 build: warn on self-assignment (Cory Fields)
53372f21767be449bb452fc3f5fe7f16286ae371 refactor: disable self-assign warning for tests (Cory Fields)
Pull request description:
Belt-and suspenders after #30234. Self-assignment should be safe _and_ discouraged.
We used to opt out of this warning because something deep in our serialization/byteswapping code could self-assign, but that doesn't appear to be the case anymore.
ACKs for top commit:
maflcko:
ACK 15796d4b61342f75548b20a18c670ed21d102ba8
fanquake:
ACK 15796d4b61342f75548b20a18c670ed21d102ba8 - not a huge fan of inline pragma usage, but this seems fine, given it's to work around an already-fixed compiler bug, and we'll only be carrying it for a shortish time in any case.
Tree-SHA512: 1f95f7c730b974ad1da55ebd381040bac312f2f380fff9d569ebab91d7c1963592a84d1613d81d96238c6f5a66aa40deebba68a76f6b24b02150d0a77c769654
|
|
This reverts cfaac2a60f3ac63ae8deccb03d88bd559449b78c
|
|
|
|
|
|
1f6ab1215bbb1f8a5f1743c3c413b95ad08090df minor: remove unnecessary semicolons from RPC content type examples (Matthew Zipkin)
b22529529823c0cb5916ac318c8536e9107b7e78 test: use json-rpc 2.0 in all functional tests by default (Matthew Zipkin)
391843b0297db03d71a8d88ab77609e2ad230bf2 bitcoin-cli: use json-rpc 2.0 (Matthew Zipkin)
d39bdf339772166a5545ae811e58b7764af093a8 test: remove unused variable in interface_rpc.py (Matthew Zipkin)
0ead71df8c83a2f9eae1220544ec84dcf38a0326 doc: update and link for JSON-RPC 2.0 (Matthew Zipkin)
Pull request description:
This is a follow-up to #27101.
- Addresses [post-merge comments ](https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1606723428)
- bitcoin-cli uses JSON-RPC 2.0
- functional tests use JSON-RPC 2.0 by default (exceptions are in the regression tests added by #27101)
ACKs for top commit:
tdb3:
ACK 1f6ab1215bbb1f8a5f1743c3c413b95ad08090df
cbergqvist:
ACK 1f6ab1215bbb1f8a5f1743c3c413b95ad08090df
Tree-SHA512: 49bf14c70464081280216ece538a2f5ec810bac80a86a83ad3284f0f1b017edf755a1a74a45be279effe00218170cafde7c2de58aed07097a95c2c6b837a6b6c
|
|
In order to ensure that the change of nVersion to a uint32_t in the
previous commit has no effect, rename nVersion to version in this commit
so that reviewers can easily spot if a spot was missed or if there is a
check somewhere whose semantics have changed.
|
|
This is a just a mechanical change, renaming and inverting the meaning
of the indexing variable.
"m_blockfiles_indexed" is a more straightforward name for this variable
because this variable just indicates whether or not
<datadir>/blocks/blk?????.dat files have been indexed in the
<datadir>/blocks/index LevelDB database. The name "m_reindexing" was
more confusing, it could be true even if -reindex was not specified, and
false when it was specified. Also, the previous name unnecessarily
required thinking about the whole reindexing process just to understand
simple checks in validation code about whether blocks were indexed.
The motivation for this change is to follow up on previous commits,
moving away from having multiple variables called "reindex" internally,
and instead naming variables individually after what they do and
represent.
|
|
Before this change continuing a reindex without the -reindex flag set
would leave the block and coins db intact, but discard the data of the
optional indexes. While not a bug per se, wiping the data again is
wasteful, both in terms of having to write it again, and potentially
leading to longer startup times.
When initially running a reindex, both the block index and any further
activated indexes are wiped. On an index's Init(), both the best block
stored by the index and the chain's tip are null. An index's m_synced
member is therefore true. This means that it will process blocks through
validation events while the reindex is running.
Currently, if the reindex is continued without the user re-specifying
the reindex flag, the block index is preserved but further index data is
wiped. This leads to the stored best block being null, but the chain tip
existing. The m_synced member will be set to false. The index will not
process blocks through the validation interface, but instead use the
background sync once the reindex is completed.
If the index is preserved (this change) after a restart its best block
may potentially match the chain tip. The m_synced member will be set to
true and the index can process validation events during the rest of the
reindex.
|
|
Drop confusing kernel options:
BlockManagerOpts::reindex
ChainstateLoadOptions::reindex
ChainstateLoadOptions::reindex_chainstate
Replacing them with more straightforward options:
ChainstateLoadOptions::wipe_block_tree_db
ChainstateLoadOptions::wipe_chainstate_db
Having two options called "reindex" which did slightly different things
was needlessly confusing (one option wiped the block tree database, and
the other caused block files to be rescanned). Also the previous set of
options did not allow rebuilding the block database without also
rebuilding the chainstate database, when it should be possible to do
those independently.
|
|
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.
|
|
30a01134cdec37e7467fcd6eee8b0ae3890a131c [doc] update bips.md for 431 (glozow)
9dbe6a03f0d6e70ccdf8e8715f888c0c17216bee [test] wallet uses CURRENT_VERSION which is 2 (glozow)
539404fe0fc0346b3aa77c330b38a5a0ad6565b2 [policy] make v3 transactions standard (glozow)
052ede75aff5c9f3a0a422ef413852eabeecc665 [refactor] use TRUC_VERSION in place of 3 (glozow)
Pull request description:
Make `nVersion=3` (which is currently nonstandard on mainnet) standard.
Note that we will treat these transactions as Topologically Restricted Until Confirmation (TRUC). Spec is in BIP 431 and implementation is in #28948, #29306, and #29873
See #27463 for overall project tracking, and #29319 for information about relevance to cluster mempool.
ACKs for top commit:
sdaftuar:
utACK 30a01134c
achow101:
ACK 30a01134cdec37e7467fcd6eee8b0ae3890a131c
instagibbs:
utACK 30a01134cdec37e7467fcd6eee8b0ae3890a131c
murchandamus:
ACK 30a01134cdec37e7467fcd6eee8b0ae3890a131c
ismaelsadeeq:
ACK 30a01134cdec37e7467fcd6eee8b0ae3890a131c 🛰️
Tree-SHA512: 2a4aec0442c860e792a061d83e36483c1f1b426f946efbdf664c8db97a596e498b535707e1d3a900218429486ea69fd4552e3d476526a6883cbd5556c6534b48
|
|
|