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
The `AC_CHECK_HEADERS` macro defines `HAVE_SYS_SYSCTL_H` if the
`sys/sysctl.h` header is found. However, in the source code, this header
is guarded by `HAVE_SYSCTL` and `HAVE_SYSCTL_ARND` macros, which have
their own checks. Since `HAVE_SYS_SYSCTL_H` is not used, we can skip the
`AC_CHECK_HEADERS(... sys/sysctl.h ...)` check.
da205dda14 ci: increase available ccache size to 300MB (Max Edwards)
4ecbbd9b7f ci: add option for running tests without volume (Max Edwards)
Pull request description:
Fixes: https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1645950272
Cache wasn't being saved when run on GHA because the default behaviour of the CI script was to store cache items in a docker volume. This works on Cirrus CI as the volumes are shared but it does not work on Github Actions in which each run is ephemeral.
Kept the default behaviour the same so hopefully this continues to work for the Cirrus CI jobs.
ACKs for top commit:
maflcko:
utACK da205dda14
hebasto:
ACK da205dda14.
Tree-SHA512: 3b35482c0628adb60574a1462181ecfcb06cb237ed48beb6fe9aa51110be82f863dc9147e7f8d82960450aa6ecc3a24a70e3c8283fd24cdad075dbfb8fc93095
This test type is represented using SEND_NO_AAD. If AAD of the first encrypted packet
sent after the garbage terminator (optional decoy packet/version packet) hasn't been
filled, disconnection happens.
This test type is represented using WRONG_GARBAGE.
Here, garbage bytes sent to TestNode are assumed to be tampered with and
do not correspond to the garbage bytes which P2PInterface calculated and
uses.
This test type is represented using WRONG_GARBAGE_TERMINATOR.
since the wrong garbage terminator is sent to TestNode, TestNode
will interpret all of the gabage bytes, wrong garbage terminator,
decoy messages and version packet it receives as garbage bytes.
If the length of all these is more than 4095 + 16, it will result
in a missing garbage terminator error. otherwise, it will result
in a V2 handshake timeout error.
Send only MAX_GARBAGE_LEN//2 bytes of garbage data to TestNode
so that the total length received by the TestNode is at max
= (MAX_GARBAGE_LEN//2) + 16 + 10*120 + 20 = 3283 bytes
(which is less than 4095 + 16 bytes) and we get a consistent
V2 handshake timeout error message.
If we do not limit the garbage length sent, we will intermittently
get both missing garbage terminator error and V2 handshake
timeout error based on the garbage length and decoy packets length
which are chosen at random.
GetFirstStoredBlock is generalized to check for any data status with a
status mask that needs to be passed as a parameter. To reflect this the
function is also renamed to GetFirstBlock.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
c67f215ea5 ci: clarify Cirrus self-hosted workers setup (Sjors Provoost)
Pull request description:
Taken from #29274 (except for two paragraphs that require the other commits in that PR).
ACKs for top commit:
maflcko:
ACK c67f215ea5
tdb3:
ACK c67f215ea5
Tree-SHA512: 321cc327bfbf0b8e55eb84cb259cf55a66d480c99abe6824248f8b5fdb9a31a079f7ce2c5a6c27afa809aa343d1efb0744a19dd379c17162b21fdf24b6b8836b
5729dbbb74 refactor: remove extraneous lock annotations from function definitions (Cory Fields)
Pull request description:
These annotations belong in the declarations rather than the definitions. While harmless now, future versions of clang may warn about these.
Discovered these using the upstream WIP: https://github.com/llvm/llvm-project/pull/67520
ACKs for top commit:
instagibbs:
ACK 5729dbbb74
maflcko:
ACK 5729dbbb74🦋
Tree-SHA512: c82c6b269dd353b140cbb36b5519ab2637e54034f159d6ad3eb78c6f4019aa053a5973c626395f0ed3366b9f4117ecc4fe7926b83e9714b1e21c97d5e4bed8d7
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
DANGER_CI_ON_HOST_CACHE_FOLDERS if set will mount caches in directories on the host rather than in docker volumes. Supports saving and restoring caches on Github Actions.
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
refactor: CBlockHeaderAndShortTxIDs constructor now always takes an explicit nonce.
test: Make blockencodings_tests deterministic using fixed seed providing deterministic
CBlockHeaderAndShortTxID nonces and dummy transaction IDs.
Fixes very rare flaky test failures, where the ShortIDs of test transactions collide, leading to
`READ_STATUS_FAILED` from PartiallyDownloadedBlock::InitData and/or `IsTxAvailable` giving `false`
when the transaction should actually be available.
* Use a new `FastRandomContext` with a fixed seed in each test, to ensure 'random' uint256s
used as fake prevouts are deterministic, so in-turn test txids and short IDs are deterministic
and don't collide causing very rare but flaky test failures.
* Add new test-only/internal initializer for `CBlockHeaderAndShortTxIDs` that takes a specified
nonce to further ensure determinism and avoid rare but undesireable short ID collisions.
In a test context this nonce is set to a fixed known-good value. Normally it is random, as
previously.
Flaky test failures can be reproduced with:
```patch
diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp
index 695e8d806a..64d635a97a 100644
--- a/src/blockencodings.cpp
+++ b/src/blockencodings.cpp
@@ -44,7 +44,8 @@ void CBlockHeaderAndShortTxIDs::FillShortTxIDSelector() const {
uint64_t CBlockHeaderAndShortTxIDs::GetShortID(const Wtxid& wtxid) const {
static_assert(SHORTTXIDS_LENGTH == 6, "shorttxids calculation assumes 6-byte shorttxids");
- return SipHashUint256(shorttxidk0, shorttxidk1, wtxid) & 0xffffffffffffL;
+ // return SipHashUint256(shorttxidk0, shorttxidk1, wtxid) & 0xffffffffffffL;
+ return SipHashUint256(shorttxidk0, shorttxidk1, wtxid) & 0x0f;
}
```
to increase the likelihood of a short ID collision; and running
```shell
set -e;
n=0;
while (( n++ < 5000 )); do
src/test/test_bitcoin --run_test=blockencodings_tests;
done
```
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 addresses of the iterator values are non-deterministic (i.e. they
depend on where the values were allocated). This causes stability issues
when fuzzing (e.g. in the `txorphan` and `mini_miner` harnesses), due
the orders (derived from IteratorComparator) not being deterministic.
Improve stability by comparing the first element in the iterator value
pair instead of using the the value addresses.
8acdf66540 upnp: add compatibility for miniupnpc 2.2.8 (Cory Fields)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/30266
Miniupnpc 2.2.8 [changed the function signature of `UPNP_GetValidIGD`](c0a50ce33e (diff-5a0d7cff00628c2c64a617edb347c0f283e3a75e7df910e7e8438fc6db23f610R122)) without taking much care with the abi :(
~This is the minimal change to cope with that. Also included in this PR is a temporary bump to 2.2.8 to verify that it builds correctly. I'm happy to revert that and discuss the bump separately, as miniupnpc bumps require some scrutiny.~
I believe that this is problematic if we build against one version and encounter a different one at runtime. This is not a problem for depends because we build statically. But for users who are self-building against shared system libs, care must be taken to run against the same version used for linking.
Some quick digging shows that at least Ubuntu/Arch make the distinction between soversions:
`libminiupnpc.so.17` -> `libminiupnpc.so.18`. So in practice, I suppose this shouldn't be much of a problem.
Boooo for the upstream loose abi policy.
ACKs for top commit:
edilmedeiros:
reACK 8acdf66540
fanquake:
ACK 8acdf66540
Tree-SHA512: d2236ec8aef57a5c879065fbbe20080a14e4bf7b44c0bf506707eb946f72aa5837aba2fb2426d6853d21a9b77db5d72561d29d7ea645714d90309e11fe11d354
Set tip at the start of the function and only update it for a long poll.
Additionally have getTipHash return an optional, so the
caller can explicitly check that a tip exists.
This makes the options argument for BlockAssembler constructor mandatory,
dropping implicit use of ArgsManager. The caller i.e. the Mining
interface implementation now handles this.
In a future Stratum v2 change the Options object needs to be
mofified after arguments have been processed. Specifically
the pool communicates how many extra bytes it needs for
its own outputs (payouts, extra commitments, etc). This will need
to be substracted from what the user set as -blockmaxweight.
Such a change can be implemented in createNewBlock, after
ApplyArgsManOptions.
