When using CMake, the user can select the MSVC runtime library to be:
1) Statically-linked (with the corresponding `x64-windows-static` vcpkg
triplet) or
2) Dynamically-linked (with the corresponding `x64-windows` vcpkg
triplet)
In the latter case, the compiler emits the C4273 warning.
As the "Necessary on some platforms" comment does not apply to MSVC,
skip the declaration for MSVC.
The body of the fuzz test should ideally be a pure function. If data is
persisted in the cache over many iterations, and there is a crash,
reproducing it from the input might be difficult.
Adds more testing options for creating an environment without networking
and a validation interface. This is useful for improving the performance
of the utxo snapshot fuzz test, which constructs a new TestingSetup on
each iteration.
The formatting of log messages isn't defined until StartLogging() is
called; so can't be correctly applied to early log messages from prior
to that call. Instead of saving the output log message, save the inputs
to the logging invocation (including time, mocktime and thread name),
and format those inputs into a log message when StartLogging() is called.
23333b7ed2 net: Allow DNS lookups on nodes with IPV6 lo only (Max Edwards)
Pull request description:
This is similar to (but does not fix) https://github.com/bitcoin/bitcoin/issues/13155 which I believe is the same issue but in libevent.
The issue is on a host that has IPV6 enabled but only a loopback IP address `-proxy=[::1]` will fail as `[::1]` is not considered valid by `getaddrinfo` with `AI_ADDRCONFIG` flag. I think the loopback interface should be considered valid and we have a functional test that will try to test this: `feature_proxy.py`.
To replicate the issue, run `feature_proxy.py` inside a docker container that has IPV6 loopback ::1 address without specifically giving that container an external IPV6 address. This should be the default with recent versions of docker. IPV6 on loopback interface was enabled in docker engine 26 and later ([https://docs.docker.com/engine/release-notes/26.0/#bug-fixes-and-enhancements-2](https://docs.docker.com/engine/release-notes/26.0/#bug-fixes-and-enhancements-2)).
`AI_ADDRCONFIG` was introduced to prevent slow DNS lookups on systems that were IPV4 only.
References:
Man section on `AI_ADDRCONFIG`:
```
If hints.ai_flags includes the AI_ADDRCONFIG flag, then IPv4 addresses are returned in the list pointed to by res only if the local system has at least one IPv4 address configured, and IPv6 addresses
are returned only if the local system has at least one IPv6 address configured. The loopback address is not considered for this case as valid as a configured address. This flag is useful on, for ex‐
ample, IPv4-only systems, to ensure that getaddrinfo() does not return IPv6 socket addresses that would always fail in connect(2) or bind(2).
```
[AI_ADDRCONFIG considered harmful Wiki entry by Fedora](https://fedoraproject.org/wiki/QA/Networking/NameResolution/ADDRCONFIG)
[Mozilla discussing slow DNS without AI_ADDRCONFIG and also localhost issues with it](https://bugzilla.mozilla.org/show_bug.cgi?id=467497)
ACKs for top commit:
achow101:
ACK 23333b7ed2
tdb3:
ACK 23333b7ed2
pinheadmz:
ACK 23333b7ed2
Tree-SHA512: 5ecd8c72d1e1c28e3ebff07346381d74eaddef98dca830f6d3dbf098380562fa68847d053c0d84cc8ed19a45148ceb5fb244e4820cf63dccb10ab3db53175020
55b6d7be68 validation: Don't load a snapshot if it's not in the best header chain. (Martin Zumsande)
Pull request description:
This was suggested by me in the discussion of #30288, which has more context.
If the snapshot is not an ancestor of the most-work header (`m_best_header`), syncing from that alternative chain leading to `m_best_header` should be prioritised. Therefore it's not useful loading the snapshot in this situation.
If the other chain turns out to be invalid or the chain with the snapshot retrieves additional headers so that it's the most-work one again (see functional test), `m_best_header` will change and loading the snapshot will be possible again.
Because of the work required to generate a conflicting headers chain, a situation with two conflicting chains should only be possible under extreme circumstances, such as major forks.
ACKs for top commit:
fjahr:
re-ACK 55b6d7be68
achow101:
ACK 55b6d7be68
alfonsoromanz:
Re ACK 55b6d7be68
Tree-SHA512: 4fbea5ab1038ae353fc949a186041cf9b397e7ce4ac59ff36f881c9437b4f22ada922490ead5b2661389eb1ca0f3d1e7e7e6a4261057678643e71594a691ac36
fac932bf93 refactor: Use util::Split to avoid a harmless unsigned-integer-overflow (MarcoFalke)
fab54db9f1 rest: Reject negative outpoint index in getutxos parsing (MarcoFalke)
Pull request description:
In `rest_getutxos` outpoint indexes such as `+N` or `-N` are accepted. This should be harmless, because any index out of range should be treated as a non-existent utxo. However, a negative index can't exist ever, so it seems better to reject all signs, whether `+` or `-`.
ACKs for top commit:
achow101:
ACK fac932bf93
hodlinator:
ut-ACK fac932bf93
tdb3:
re ACK fac932bf93
danielabrozzoni:
ACK fac932bf93
brunoerg:
reACK fac932bf93
Tree-SHA512: 8f1a75248cb61e1c4beceded6ed170db83b07f30fbcf93a26acfffc00ec4546572366eff87907a7e1423d7d3a2a9e57a0a7a9bacb787c86463f842d7161c16bc
c504b6997b refactor: add coinbase constraints to BlockCreateOptions (Sjors Provoost)
6b4c817d4b refactor: pass BlockCreateOptions to createNewBlock (Sjors Provoost)
323cfed595 refactor: use CHECK_NONFATAL to avoid single-use symbol (Sjors Provoost)
Pull request description:
When generating a block template through e.g. getblocktemplate RPC, we reserve 4000 weight units and 400 sigops. Pools use this space for their coinbase outputs.
At least one pool patched their Bitcoin Core node to adjust these hardcoded values. They eventually [produced an invalid block](https://bitcoin.stackexchange.com/questions/117837/how-many-sigops-are-in-the-invalid-block-783426) which exceeded the sigops limit.
The existince of such patches suggests it may be useful to make this value configurable. This PR would make such a change easier. However, the main motivation is that in the Stratum v2 spec requires the pool to communicate the maximum bytes they intend
to add to the coinbase outputs.
Specifically the `CoinbaseOutputDataSize` message which is part of the [Template Distribution Protocol](https://github.com/stratum-mining/sv2-spec/blob/main/07-Template-Distribution-Protocol.md#71-coinbaseoutputdatasize-client---server) has a field `coinbase_output_max_additional_size`.
A proposed change to the spec adds the max additional sigops as well: https://github.com/stratum-mining/sv2-spec/pull/86. Whether that change makes it into the spec is not important though, as adding both to `BlockAssembler::Options` makes sense.
The first commit is a test refactor followup for #30335, related to the code that's changed here, but not required.
The second commit introduces BlockCreateOptions, with just `use_mempool`.
The thirds commit adds `coinbase_max_additional_weight` and `coinbase_output_max_additional_sigops` to `BlockCreateOptions`. They use the originally hardcoded values, and no existing caller overrides these defaults. This changes in #29432.
ACKs for top commit:
itornaza:
tested ACK c504b6997b
ryanofsky:
Code review ACK c504b6997b
ismaelsadeeq:
Code review ACK c504b6997b
Tree-SHA512: de2fa085f47048c91d95524e03f909f6f27f175c1fefa3d6106445e7eb5cf5b710eda6ea5b641cf3b4704a4e4e0181a0c829003b9fd35465f2a46167e5d64487
44f08786f4 test: Fix MSVC warning C4101 "unreferenced local variable" (Hennadii Stepanov)
5d25a82b9a univalue, refactor: Convert indentation tabs to spaces (Hennadii Stepanov)
Pull request description:
This PR is split from https://github.com/bitcoin/bitcoin/pull/30454 and addresses MSVC warning [C4101](https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4101) "unreferenced local variable". The current MSVC build system in the master branch skips building univalue tests, so it is not affected.
No behaviour changes.
ACKs for top commit:
kevkevinpal:
utACK [44f0878](44f08786f4)
maflcko:
ACK 44f08786f4
theuni:
trivial ACK 44f08786f4.
Tree-SHA512: 661d3b40ddb4f7915de7a65ccb27a24da88ae499ce03c036099007260b0597e83738f1a3a420985b51f798ee309ade32988c6d78f4ffed401099b175a0b2025b
734076c6de [wallet, rpc]: add `max_tx_weight` to tx funding options (ismaelsadeeq)
b6fc5043c1 [wallet]: update the data type of `change_output_size`, `change_spend_size` and `tx_noinputs_size` to `int` (ismaelsadeeq)
baab0d2d43 [doc]: update reason for deducting change output weight (ismaelsadeeq)
7f61d31a5c [refactor]: update coin selection algorithms input parameter `max_weight` name (ismaelsadeeq)
Pull request description:
This PR taken over from #29264
The PR added an option `max_tx_weight` to transaction funding RPC's that ensures the resulting transaction weight does not exceed the specified `max_tx_weight` limit.
If `max_tx_weight` is not given `MAX_STANDARD_TX_WEIGHT` is used as the max threshold.
This PR addressed outstanding review comments in #29264
For more context and rationale behind this PR see https://delvingbitcoin.org/t/lightning-transactions-with-v3-and-ephemeral-anchors/418/11?u=instagibbs
ACKs for top commit:
achow101:
ACK 734076c6de
furszy:
utACK 734076c6de
rkrux:
reACK [734076c](734076c6de)
Tree-SHA512: 013501aa443d239ee2ac01bccfc5296490c27b4edebe5cfca6b96c842375e895e5cfeb5424e82e359be581460f8be92095855763a62779a18ccd5bdfdd7ddce7
When generating a block template through e.g. getblocktemplate RPC,
we reserve 4000 weight units and 400 sigops. Pools use this space
for their coinbase outputs.
At least one pool patched their Bitcoin Core node to adjust
these hardcoded values. They eventually produced an invalid
block which exceeded the sigops limit.
https://bitcoin.stackexchange.com/questions/117837/how-many-sigops-are-in-the-invalid-block-783426
The existince of such patches suggests it may be useful to
make this value configurable. This commit would make such a
change easier.
The main motivation however is that the Stratum v2 spec
requires the pool to communicate the maximum bytes they intend
to add to the coinbase outputs. A proposed change to the spec
would also require them to communicate the maximum number of sigops.
This commit also documents what happens when
-blockmaxweight is lower than the coinbase
reserved value.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
d440f13db0 crypto: Guard code with `ENABLE_SSE41` macro (Hennadii Stepanov)
6ec1ca7c85 build: Fix test for SSE4.1 intrinsics (Hennadii Stepanov)
Pull request description:
1. Fix the test for SSE4.1 intrinsics during build system configuration, which currently can be false positive, for example, when `CXXFLAGS="-mno-sse4.1"` provided.
This PR fixes the test by adding the `_mm_blend_epi16` SSE4.1 function used in our codebase.
2. Guard `sha_x86_shani.cpp` code with `ENABLE_SSE41` macro as it uses the `_mm_blend_epi16` function from
the SSE4.1 instruction set.
It is possible that SHA-NI is enabled even when SSE4.1 is disabled, which causes compile errors in the master branch.
Closes https://github.com/bitcoin/bitcoin/issues/28864.
ACKs for top commit:
sipa:
utACK d440f13db0
willcl-ark:
tACK d440f13db0
theuni:
utACK d440f13db0
Tree-SHA512: a6e1e8c94e1b94874ff51846815ef445e6135cbdb01b08eb695b3548115f2340dd835ebe53673ae46a553fe6be4815e68d8642c34235dd7af5106c4b7c9ea6f3
fa927055dd refactor: Make m_last_notified_header private (MarcoFalke)
Pull request description:
Seems brittle to expose mutable fields public.
Fix it by making it private.
Fixes https://github.com/bitcoin/bitcoin/pull/30425#discussion_r1677633601
ACKs for top commit:
dergoegge:
utACK fa927055dd
Tree-SHA512: d9841c42571144ced0edeaa4bb1d96a177a011dca37c8342c66513477c37278602a1b88beb93068b94fc4443b1552c8fc9f98bcf0bda7d0fc101e61e90c33944
fa6390df20 doc: getaddressinfo[isscript] is optional (MarcoFalke)
Pull request description:
`isscript` is unknown for unknown witness versions, so it should be marked optional in the docs
Fixes https://github.com/bitcoin/bitcoin/issues/30456
ACKs for top commit:
stickies-v:
ACK fa6390df20
tdb3:
ACK fa6390df20
Tree-SHA512: f728f18e0871923225e0bf29594f8095997456cf55409f42087b5f70f95bef10f984323b48d2b484b6705f23b04e9e8a3fe42446830638fdd70453c18fd7f189
The previous commit added a test which would fail the
unsigned-integer-overflow sanitizer. The warning is harmless and can be
triggered on any commit, since the code was introduced.
For reference, the warning would happen when the separator `-` was not
present.
For example:
GET /rest/getutxos/6a297bfa5cb8dd976ab0207a767d6cbfaa5e876f30081127ec8674c8c52b16c0_+1.json
would result in:
rest.cpp:792:77: runtime error: unsigned integer overflow: 18446744073709551615 + 1 cannot be represented in type 'size_type' (aka 'unsigned long')
#0 0x55ad42c16931 in rest_getutxos(std::any const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&) src/rest.cpp:792:77
#1 0x55ad4319e3c0 in std::function<bool (HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&)>::operator()(HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9
#2 0x55ad4319e3c0 in HTTPWorkItem::operator()() src/httpserver.cpp:59:9
#3 0x55ad431a3eea in WorkQueue<HTTPClosure>::Run() src/httpserver.cpp:114:13
#4 0x55ad4318f961 in HTTPWorkQueueRun(WorkQueue<HTTPClosure>*, int) src/httpserver.cpp:403:12
#5 0x7f078ebcbbb3 (/lib/x86_64-linux-gnu/libstdc++.so.6+0xeabb3) (BuildId: 40b9b0d17fdeebfb57331304da2b7f85e1396ef2)
#6 0x55ad4277e01c in asan_thread_start(void*) asan_interceptors.cpp.o
#7 0x7f078e840a93 (/lib/x86_64-linux-gnu/libc.so.6+0x9ca93) (BuildId: 08134323d00289185684a4cd177d202f39c2a5f3)
#8 0x7f078e8cdc3b (/lib/x86_64-linux-gnu/libc.so.6+0x129c3b) (BuildId: 08134323d00289185684a4cd177d202f39c2a5f3)
SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow rest.cpp:792:77
7e36dca657 test: add test for modififed walletprocesspsbt calls (willcl-ark)
39cea21ec5 wallet: fix FillPSBT errantly showing as complete (willcl-ark)
Pull request description:
Fixes: #30077
Fix cases of calls to `FillPSBT` returning `complete=true` when it's not
the case.
This can happen when some inputs have been signed but the transaction is
subsequently modified, e.g. in the context of PayJoins.
Also fixes a related bug where a finalized hex string is attempted to be
added during `walletprocesspsbt` but a CHECK_NONFATAL causes an abort.
ACKs for top commit:
achow101:
ACK 7e36dca657
ismaelsadeeq:
Tested ACK 7e36dca657
pinheadmz:
re-ACK 7e36dca657
Tree-SHA512: e35d19789899c543866d86d513506494d672e4bed9aa36a995dbec4e72f0a8ec5536b57c4a940a18002ae4a8efd0b007c77ba64e57cd52af98e4ac0e7bf650d6
bca346a970 net: require P2P binds to succeed (Vasil Dimov)
af552534ab net: report an error if unable to bind on the Tor listening addr:port (Vasil Dimov)
9a7e5f4d68 net: don't extra bind for Tor if binds are restricted (Vasil Dimov)
Pull request description:
Make it possible to disable the Tor binding on `127.0.0.1:8334` and stop startup if any P2P bind fails instead of "if all P2P binds fail".
Fixes https://github.com/bitcoin/bitcoin/issues/22726
Fixes https://github.com/bitcoin/bitcoin/issues/22727
ACKs for top commit:
achow101:
ACK bca346a970
cbergqvist:
ACK bca346a970
pinheadmz:
ACK bca346a970
Tree-SHA512: fabef89a957191eea4f3e3b6109d2b8389f27ecc74440a920b0c10f31fff00a85bcfd1eb3c91826c7169c618f4de8a8d0a260e2caf40fd854f07ea9a980d8603
fe92c15f0c script/sign: avoid duplicated signature verification after signing (Sebastian Falbesoner)
080089567c bench: add benchmark for `SignTransaction` (Sebastian Falbesoner)
Pull request description:
This PR is a small performance improvement on the `SignTransaction` function, which is used mostly by the wallet (obviously) and the `signrawtransactionwithkey` RPC. The lower-level function `ProduceSignature` already calls `VerifyScript` internally as last step in order to check whether the signature data is complete:
daa56f7f66/src/script/sign.cpp (L568-L570)
If and only if that is the case, the `complete` field of the `SignatureData` is set to `true` accordingly and there is no need then to verify the script after again, as we already know that it would succeed.
This leads to a rough ~20% speed-up for `SignTransaction` for single-input ECDSA or Taproot transactions, according to the newly introduced `SignTransaction{ECDSA,Taproot}` benchmarks:
```
$ ./src/bench/bench_bitcoin --filter=SignTransaction.*
```
without commit 18185f4f578b8795fdaa75926630a691e9c8d0d4:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 185,597.79 | 5,388.00 | 1.6% | 0.22 | `SignTransactionECDSA`
| 141,323.95 | 7,075.94 | 2.1% | 0.17 | `SignTransactionSchnorr`
with commit 18185f4f578b8795fdaa75926630a691e9c8d0d4:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 149,757.86 | 6,677.45 | 1.4% | 0.18 | `SignTransactionECDSA`
| 108,284.40 | 9,234.94 | 2.0% | 0.13 | `SignTransactionSchnorr`
Note that there are already signing benchmarks in the secp256k1 library, but `SignTransaction` does much more than just the cryptographical parts, i.e.:
* calculate the unsigned tx's `PrecomputedTransactionData` if necessary
* apply Solver on the prevout scriptPubKey, fetch the relevant keys from the signing provider
* perform the actual signing operation (for ECDSA signatures, that could be more than once due to low-R grinding)
* verify if the signatures are correct by calling `VerifyScript` (more than once currently, which is fixed by this PR)
so it probably makes sense to also have benchmarks from that higher-level application perspective.
ACKs for top commit:
achow101:
ACK fe92c15f0c
furszy:
utACK fe92c15f0c
glozow:
light review ACK fe92c15f0c
Tree-SHA512: b7225ff9e8a640ca5222dea5b2a463a0f9b9de704e4330b5b9a7bce2d63a1f4620575c474a8186f4708d7d9534eab55d000393d99db79c0cfc046b35f0a4a778
fa6270737e rpc: Use CHECK_NONFATAL over Assert (MarcoFalke)
Pull request description:
Any RPC method should not abort the whole node when an internal logic error happens.
Fix it by just aborting this single RPC method call when an error happens.
Also, fix the linter to find the fixed cases.
ACKs for top commit:
achow101:
ACK fa6270737e
stickies-v:
ACK fa6270737e
tdb3:
ACK fa6270737e
hodlinator:
ACK fa6270737e
Tree-SHA512: dad2f31b01a66578949009499e4385fb4d72f0f897419f2a6e0ea02e799b9a31e6ecb5a67fa5d27fcbc7939fe8acd62dc04e877b35831493b7f2c604dec7dc64
5fd4836019 init: change shutdown order of load block thread and scheduler (Martin Zumsande)
Pull request description:
This avoids situations during a reindex, in which the shutdown doesn't finish since `LimitValidationInterfaceQueue()` is called by the load block thread when the scheduler is already stopped, in which case it would block indefinitely. This can lead to intermittent failures in `feature_reindex.py` (#30424), which I could locally reproduce with
```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 74f0e4975c..be1706fdaf 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3446,6 +3446,7 @@ static void LimitValidationInterfaceQueue(ValidationSignals& signals) LOCKS_EXCL
AssertLockNotHeld(cs_main);
if (signals.CallbacksPending() > 10) {
+ std::this_thread::sleep_for(std::chrono::milliseconds(50));
signals.SyncWithValidationInterfaceQueue();
}
}
```
It has also been reported by users running `reindex-chainstate` (#23234).
I thought for a bit about potential downsides of changing this order, but couldn't find any.
Fixes #30424
Fixes #23234
ACKs for top commit:
maflcko:
review ACK 5fd4836019
hebasto:
re-ACK 5fd4836019.
tdb3:
ACK 5fd4836019
BrandonOdiwuor:
Code Review ACK 5fd4836019
Tree-SHA512: 3b8894e99551c5d4392b55eaa718eee05841a7287aeef2978699e1d633d5234399fa2f5a3e71eac1508d97845906bd33e0e63e5351855139e7be04c421359b36
51fa26239a refactor: Mark some static global vars as const (TheCharlatan)
39f9b80fba refactor: De-globalize last notified header index (TheCharlatan)
3443943f86 refactor: De-globalize validation benchmark timekeeping (TheCharlatan)
Pull request description:
In future, users of the kernel library might run multiple chainstates in parallel, or create and destroy multiple chainstates over the lifetime of a process. Having static, mutable variables could lead to state inconsistencies in these scenarios.
---
This pull request is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587).
ACKs for top commit:
dergoegge:
Code review ACK 51fa26239a
maflcko:
ACK 51fa26239a🍚
tdb3:
code review ACK 51fa26239a
Tree-SHA512: da91aa7ffa343325cabb8764ef03c8358845662cf0ba8a6cc1dd38e40e5462d88734f2b459c2de8e7a041551eda9143d92487842609f7f30636f61a0cd3c57ee
8607773750 Add fuzz test for FSChaCha20Poly1305 (stratospher)
c807f33228 Add fuzz test for AEADChacha20Poly1305 (stratospher)
Pull request description:
This PR adds fuzz tests for `AEADChaCha20Poly1305` and `FSChaCha20Poly1305` introduced in #28008.
Run using:
```
$ FUZZ=crypto_aeadchacha20poly1305 src/test/fuzz/fuzz
$ FUZZ=crypto_fschacha20poly1305 src/test/fuzz/fuzz
```
ACKs for top commit:
dergoegge:
tACK 8607773750
marcofleon:
Tested ACK 8607773750. Ran both targets for ~200 CPU hours. Coverage of intended targets looks good to me. The simulation of damaged keys and checks that follow seem useful as well.
Tree-SHA512: b6b85661d896e653caeed330f941fde665fc2bbd97ecd340808a3f365c469fe9134aa77316569a771dc36d1158cac1a5f76700bcfc45fff12aef07562e48feb9
Resetting m_recent_rejects once per block is more efficient than
comparing hashRecentRejectsChainTip with the chain tip every time we
call AlreadyHaveTx. We keep hashRecentRejectsChainTip for now to assert
that updates happen correctly; it is removed in the next commit.
This is a synchronous callback notifying clients of all tip changes.
It allows clients to respond to a new block immediately after it is
connected. The synchronicity is important for things like
m_recent_rejects, in which a transaction's validity can change (rejected
vs accepted) when this event is processed. For example, the transaction
might have a timelock condition that has just been met. This is distinct
from something like m_recent_confirmed_transactions, in which the
validation outcome is the same (valid vs already-have), so it does not
need to be reset immediately.
We need to synchronize between various tx download structures.
TxRequest does not inherently need cs_main for synchronization, and it's
not appropriate to lock all of the tx download logic under cs_main.
16bd283b3a Reapply "test: p2p: check that connecting to ourself leads to disconnect" (Sebastian Falbesoner)
0dbcd4c148 net: prevent sending messages in `NetEventsInterface::InitializeNode` (Sebastian Falbesoner)
66673f1c13 net: fix race condition in self-connect detection (Sebastian Falbesoner)
Pull request description:
This PR fixes a recently discovered race condition in the self-connect detection (see #30362 and #30368).
Initiating an outbound network connection currently involves the following steps after the socket connection is established (see [`CConnman::OpenNetworkConnection`](bd5d1688b4/src/net.cpp (L2923-L2930)) method):
1. set up node state
2. queue VERSION message (both steps 1 and 2 happen in [`InitializeNode`](bd5d1688b4/src/net_processing.cpp (L1662-L1683)))
3. add new node to vector `m_nodes`
If we connect to ourself, it can happen that the sent VERSION message (step 2) is received and processed locally *before* the node object is added to the connection manager's `m_nodes` vector (step 3). In this case, the self-connect remains undiscovered, as the detection doesn't find the outbound peer in `m_nodes` yet (see `CConnman::CheckIncomingNonce`).
Fix this by swapping the order of 2. and 3., by taking the `PushNodeVersion` call out of `InitializeNode` and doing that in the `SendMessages` method instead, which is only called for `CNode` instances in `m_nodes`.
The temporarily reverted test introduced in #30362 is readded. Fixes #30368.
Thanks go to vasild, mzumsande and dergoegge for suggestions on how to fix this (see https://github.com/bitcoin/bitcoin/issues/30368#issuecomment-2200625017 ff. and https://github.com/bitcoin/bitcoin/pull/30394#discussion_r1668290789).
ACKs for top commit:
naiyoma:
tested ACK [16bd283b3a), built and tested locally, test passes successfully.
mzumsande:
ACK 16bd283b3a
tdb3:
ACK 16bd283b3a
glozow:
ACK 16bd283b3a
dergoegge:
ACK 16bd283b3a
Tree-SHA512: 5b8aced6cda8deb38d4cd3fe4980b8af505d37ffa0925afaa734c5d81efe9d490dc48a42e1d0d45dd2961c0e1172a3d5b6582ae9a2d642f2592a17fbdc184445
Rather than pass options individually to createNewBlock and then
combining them into BlockAssembler::Options, this commit introduces
BlockCreateOptions and passes that instead.
Currently there's only one option (use_mempool) but the next
commit adds more.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
fa14e1d9d5 log: Fix __func__ in LogError in blockstorage module (MarcoFalke)
fad59a2f0f log: LogError with FlatFilePos in UndoReadFromDisk (MarcoFalke)
aaaa3323f3 refactor: Mark IsBlockPruned const (MarcoFalke)
Pull request description:
These errors should never happen in normal operation. If they do,
knowing the `FlatFilePos` may be useful to determine if data corruption
happened. Also, handle the error `pos.IsNull()` as part of `OpenUndoFile`,
because it may as well have happened due to data corruption.
This mirrors the `LogError` behavior from `ReadBlockFromDisk`.
Also, two other fixup commits in this module.
ACKs for top commit:
kevkevinpal:
ACK [fa14e1d](fa14e1d9d5)
tdb3:
cr and light test ACK fa14e1d9d5
ryanofsky:
Code review ACK fa14e1d9d5. This should make logging clearer and more consistent
Tree-SHA512: abb492a919b4796698d1de0a7874c8eae355422b992aa80dcd6b59c2de1ee0d2949f62b3cf649cd62892976fee640358f7522867ed9d48a595d6f8f4e619df50
fa690c8e53 test: [refactor] Pass TestOpts (MarcoFalke)
Pull request description:
Currently optional test context setup settings are passed by adding a new optional argument to the constructors. For example `extra_args`. This is problematic, because:
* Adding more optional settings in the future requires touching all affected constructors, increasing their verbosity.
* Setting only a later option requires setting the earlier ones.
* Clang-tidy named args passed to `std::make_unique` are not checked.
Fix all issues by adding a new struct `TestOpts`, which holds all options. Notes:
* The chain type is not an option in the struct for now, because the default values vary.
* The struct holds all possible test options globally. Not all fields may be used by all constructors. Albeit harmless, it is up to the test author to not set a field that is unused.
ACKs for top commit:
kevkevinpal:
utACK [fa690c8](fa690c8e53)
dergoegge:
utACK fa690c8e53
TheCharlatan:
Nice, ACK fa690c8e53
Tree-SHA512: 8db8efa5dff854a73757d3f454f8f902e41bb4358f5f9bae29dbb3e251e20ee93489605de51d0822ba31d97835cd15526a29c075278dd6a8bbde26134feb4f49
bc34bc2888 fuzz: limit the number of nested wrappers in descriptors (Antoine Poinsot)
8d7340105f fuzz: limit the number of sub-fragments per fragment for descriptors (Antoine Poinsot)
Pull request description:
Some of the logic in the miniscript module is quadratic. It only becomes an issue for very large uninteresting descriptors (like a `thresh` with 130k sub-fragments or a fragment with more than 60k nested `j:` wrappers).
This PR fixes the two types of fuzz timeouts reported by Marco in https://github.com/bitcoin/bitcoin/issues/28812 by trying to pinpoint the problematic descriptors through a simple analysis of the string, without limiting the size of the string itself. This is the same approach as was adopted for limiting the depth of derivation paths.
ACKs for top commit:
dergoegge:
utACK bc34bc2888
stickies-v:
Light ACK bc34bc2888
marcofleon:
Code review ACK bc34bc2888. The added comments are useful, thanks for those. Tested on the three inputs in https://github.com/bitcoin/bitcoin/issues/28812 that caused the timeouts.
Tree-SHA512: 8811c7b225684c5ecc1eb1256cf39dfa60d4518161e70210086c8a01b38927481ebe747af86aa5f4803187672d43fadabcfdfbf4e3b049738d629a25143f0e77
b71bfd9eef GUI/OptionsDialog: Prefer to stretch actual options area rather than waste space (Luke Dashjr)
Pull request description:
ACKs for top commit:
hebasto:
ACK b71bfd9eef
Tree-SHA512: b706a07292fe81379e303f9069fca6efd5ceb15ee5bb77c6aeddbf63f736494ce877b76767ff17d7becf98d07209e51c74bdb99365596b7b9f4904a30438d72d
4383dc90ba fuzz: fix key size in crypter target (brunoerg)
Pull request description:
Fixes #30251
This PR:
1. Limits `cipher_text_ed` and `random_string` (`SecureString`) size.
2. Replace `ConsumeRandomLengthByteVector` for keys to `ConsumeFixedLengthByteVector` with `WALLET_CRYPTO_KEY_SIZE`.
3. Replace `ConsumeRandomLengthByteVector` for `chSalt` to `ConsumeFixedLengthByteVector` with `WALLET_CRYPTO_SALT_SIZE`.
ACKs for top commit:
marcofleon:
Tested ACK 4383dc90ba. I ran this:
dergoegge:
utACK 4383dc90ba
Tree-SHA512: 6f09cca0b4627f49152b685ac03659c01004f2131c6aada7654606ea01f6619b1611b1d17624d2cddce277c1afdddda5f656d99f6ca8f72a22f5c0541762c964
992b1bbd5d qt: keep focus on "Hide" while ModalOverlay is visible (Jadi)
Pull request description:
During the initial sync, the Tab moves the focus to the widgets of the main window, even when the ModalOverlay is visible. This creates some weird rectangular *selections on the screen*.
This PR fixes this by keeping the focus on the "Hide" button while the ModalOverlay is visible.
Fixes #783
ACKs for top commit:
pablomartin4btc:
Concept & approach ACK 992b1bbd5d
hebasto:
re-ACK 992b1bbd5d
Tree-SHA512: f702a3fd51db4bc10780bccf76394e35a6b5fb45db72c9c23cd10d777106b08c61077d2d989003838921e76d2cb44f809399f31df76448e4305a6c2a71b5c6a3
