e3dc64f499 build: add -Wundef (fanquake)
82b43955f7 refactor: use #ifdef HAVE_SOCKADDR_UN (fanquake)
40cd7585a0 randomenv: use ifdef over if (fanquake)
7839503b30 zmq: use #ifdef ENABLE_ZMQ (fanquake)
79e197b175 build: Suppress warnings from boost and capnproto in multiprocess code (Ryan Ofsky)
Pull request description:
Turn on `-Wundef`.
[> Warn if an undefined identifier is evaluated in an #if directive. Such identifiers are replaced with zero.](https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wundef).
Note that this is still beneficial with CMake, and may even be nice to have enabled prior, to catch any change in behaviour.
If we end up with this enabled, it should probably be enough to fix#16419.
ACKs for top commit:
hebasto:
ACK e3dc64f499, I have reviewed the code and it looks OK.
Tree-SHA512: 73436ead07f3a09ba0d30f7105df50d9b2ec8452f11e866bc1c7ebc10c005772ee77fedaa125f444175663c04dfc472f98c2699c63711da356089b66a8cc3e0a
randomenv.cpp:48:5: warning: 'HAVE_VM_VM_PARAM_H' is not defined, evaluates to 0 [-Wundef]
randomenv.cpp:51:5: warning: 'HAVE_SYS_RESOURCES_H' is not defined, evaluates to 0 [-Wundef]
randomenv.cpp:424:5: error: 'HAVE_SYSCTL' is not defined, evaluates to 0 [-Werror,-Wundef]
Without this change there are errors from boost like:
/ci_container_base/depends/i686-pc-linux-gnu/include/boost/signals2/expired_slot.hpp:23:28: error: 'what' overrides a member function but is not marked 'override' [-Werror,-Wsuggest-override]
/ci_container_base/depends/i686-pc-linux-gnu/include/boost/signals2/detail/signal_template.hpp:750:32: error: 'lock_pimpl' overrides a member function but is not marked 'override' [-Werror,-Wsuggest-override]
/ci_container_base/depends/i686-pc-linux-gnu/include/boost/signals2/connection.hpp:150:22: error: 'connected' overrides a member function but is not marked 'override' [-Werror,-Wsuggest-override]
There do not seem to be errors from capnproto currently, but add a suppression
for it, too, to be consistent with other libraries.
1245d1388b netbase: extend CreateSock() to support creating arbitrary sockets (Vasil Dimov)
Pull request description:
Allow the callers of `CreateSock()` to pass all 3 arguments to the `socket(2)` syscall. This makes it possible to create sockets of any domain/type/protocol. In addition to extending arguments, some extra safety checks were put in place.
The need for this came up during the discussion in https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1618837102
ACKs for top commit:
achow101:
ACK 1245d1388b
tdb3:
re ACK 1245d1388b
theStack:
re-ACK 1245d1388b
Tree-SHA512: cc86b56121293ac98959aed0ed77812d20702ed7029b5a043586f46e74295779c5354bb0d5f9e80be6c29e535df980d34c1dbf609064fb7ea3e5ca0f0ed54d6b
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
fa7bc9bbca fuzz: Fix wallet_bdb_parser 32-bit unhandled fseek error (MarcoFalke)
Pull request description:
`std::fseek` on 64-bit past the end of the file may work fine (the following read would fail). However, on 32-bit it may fail early.
Fix it, by ignoring the error, treating it similar to a read error.
This was found by OSS-Fuzz.
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=69414
ACKs for top commit:
TheCharlatan:
ACK fa7bc9bbca
brunoerg:
utACK fa7bc9bbca
Tree-SHA512: 7a752a005837bae6846ce315a7b3b1a5fe1f440c7797c750f2c0bbb20b1ef1537cd390c425747c0c85d012655e2f908bd300ea084f82e5ada19badbf826e1ec9
fa9cb101cf refactor: Add explicit cast to expected_last_page to silence fuzz ISan (MarcoFalke)
Pull request description:
Fixes#30247
I don't think this implicit cast can lead to any bugs, so make it explicit to silence the fuzz integer sanitizer.
Can be tested with:
```
FUZZ=wallet_bdb_parser UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./src/test/fuzz/fuzz /tmp/1376869be72eebcc87fe737020add634b1a29533
```
After downloading the raw fuzz input from 1376869be7
ACKs for top commit:
dergoegge:
utACK fa9cb101cf
Tree-SHA512: 226dcc58be8d70b4eec1657f232c9c6648b5dac5eb2706e7390e65ce0a031fbaf8afce97d71a535c8294467dca4757c96f294d8cc03d5e6a1c0a036b0e070325
4ccb3d6d0d fuzz: have package_rbf always make small txns (Greg Sanders)
Pull request description:
hopefully resolves https://github.com/bitcoin/bitcoin/issues/30241
The fuzz target is generating a large amount of
transactions, but the core of the logic is
ConsumeTxMemPoolEntry making the mempool
entries for adding to the mempool. Since
ConsumeTxMemPoolEntry generates its own transaction "vsize",
we can improve efficiency of the target
by explicitly creating very small transactions,
reducing the hashing and memory burden.
ACKs for top commit:
maflcko:
lgtm ACK 4ccb3d6d0d
hodlinator:
ACK 4ccb3d6d0d
glozow:
ACK 4ccb3d6d0d
Tree-SHA512: 5d2e7e98460c6144dfe7deac554865e2e8e0e5f934dbdf5857dc4b4f471a64dc933297dc0dcf516f748a4348be6bd184808b7ece17ce073fdcc77f81b74c64de
The fuzz target is generating a large amount of
transactions, but the core of the logic is
ConsumeTxMemPoolEntry making the mempool
entries for adding to the mempool. Since
ConsumeTxMemPoolEntry generates its own transaction
"vsize", we can improve efficiency of the target
by explicitly creating very small transactions,
reducing the hashing and memory burden.
See: c0a50ce33e
The return value of 2 now indicates:
"A valid connected IGD has been found but its IP address is reserved (non routable)"
We continue to ignore any return value other than 1.
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
260f8da71a refactor: remove warnings globals (stickies-v)
9c4b0b7ce4 node: update uiInterface whenever warnings updated (stickies-v)
b071ad9770 introduce and use the generalized `node::Warnings` interface (stickies-v)
20e616f864 move-only: move warnings from common to node (stickies-v)
bed29c481a 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 260f8da71a
ryanofsky:
Code review ACK 260f8da71a. Only change since last review was rebasing
TheCharlatan:
Re-ACK 260f8da71a
Tree-SHA512: a3fcedaee0d3ad64e9c111aeb30665162f98e0e72acd6a70b76ff2ddf4f0a34da4f97ce353c322a1668ca6ee4d8a81cc6e6d170c5bbeb7a43cffdaf66646b588
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
fae3a1f006 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 fae3a1f006, 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](fae3a1f006)
achow101:
ACK fae3a1f006
Tree-SHA512: 3f99fd25d5a204d570a42d8fb2b450439aad7685692f9594cc813d97253c4df172a6ff3cf818959bfcf25dfcf8ee9a9c9ccc6028fcfcecdb47591e18c77ef246
Allow the callers of `CreateSock()` to pass all 3 arguments to the
`socket(2)` syscall. This makes it possible to create sockets of
any domain/type/protocol.
a37778d4d3 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 95812d912b
Tree-SHA512: 3d943695a3d33816cf5558b183f5629aa92a500a1544eecedf84952e93c8592a8cf0d554b88281fc0bad3c9e920ebcff1ed8edc12f8e73f36ed5335482beb829
07f64177a4 Reduce memory copying operations in bech32 encode (Lőrinc)
d5ece3c4b5 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 07f64177a4
sipa:
utACK 07f64177a4
achow101:
ACK 07f64177a4
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>
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.
c7376babd1 doc: Clarify distinction between util and common libraries in libraries.md (Ryan Ofsky)
4f74c59334 util: Move util/string.h functions to util namespace (Ryan Ofsky)
4d05d3f3b4 util: add TransactionError includes and namespace declarations (Ryan Ofsky)
680eafdc74 util: move fees.h and error.h to common/messages.h (Ryan Ofsky)
02e62c6c9a common: Add PSBTError enum (Ryan Ofsky)
0d44c44ae3 util: move error.h TransactionError enum to node/types.h (Ryan Ofsky)
9bcce2608d util: move spanparsing.h to script/parsing.h (Ryan Ofsky)
6dd2ad4792 util: move spanparsing.h Split functions to string.h (Ryan Ofsky)
23cc8ddff4 util: move HexStr and HexDigit from util to crypto (TheCharlatan)
6861f954f8 util: move util/message to common/signmessage (Ryan Ofsky)
cc5f29fbea build: move memory_cleanse from util to crypto (Ryan Ofsky)
5b9309420c build: move chainparamsbase from util to common (Ryan Ofsky)
ffa27af24d 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 c7376babd1
TheCharlatan:
Re-ACK c7376babd1
hebasto:
re-ACK c7376babd1.
Tree-SHA512: 5bcef16c1255463b1b69270548711e7ff78ca0dd34e300b95e3ca1ce52ceb34f83d9ddb2839e83800ba36b200de30396e504bbb04fa02c6d0c24a16d06ae523d
faa41e29d5 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 faa41e29d5
Tree-SHA512: 54bcaf51c83a1b48739cd7f1e8445c6eba0eb04231bce5c35591a47dddb3890ffcaf562cf932930443c80ab0e66950c4619560e6692240de0c52aeef3214facd
193c748e44 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 193c748e44
brunoerg:
ACK 193c748e44
vasild:
ACK 193c748e44
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
f51da34ec1 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 f51da34ec1
glozow:
ACK f51da34ec1
sipa:
utACK f51da34ec1
Tree-SHA512: bf96910abe9aaddd8586e6cc8f68a9bbac4c26d976ebeebcfa86b86c0da5783c1cbdbc7fa09b62cdcfde19e6442eb65a66bf1e2e80408d68e9dd9689dc22b0fa
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
5bc2077e8f validation: allow to specify frequency for -checkblockindex (Martin Zumsande)
d5a631b959 validation: improve performance of CheckBlockIndex (Martin Zumsande)
32c80413fd 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 5bc2077e8f
furszy:
ACK 5bc2077e8f
ryanofsky:
Code review ACK 5bc2077e8f. Just added suggested assert and simplification since last review
Tree-SHA512: 6b9c3e3e5069d6152b45a09040f962380d114851ff0f9ff1771cf8cad7bb4fa0ba25cd787ceaa3dfa5241fb249748e2ee6987af0ccb24b786a5301b2836f8487
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
09ef322acc [[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 09ef322acc . 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 09ef322acc
ryanofsky:
Code review ACK 09ef322acc. Just fuzz test error checking fix and updated comment since last review
Tree-SHA512: eb3361411c2db70be17f912e3b14d9cb9c60fb0697a1eded952c3b7e8675b7d783780d45c52e091931d1d80fe0f0280cee98dd57a3100def13af20259d9d1b9e
