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#30424Fixes#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
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
09370529fb fuzz: mini_miner_selection fixups. (glozow)
de273d5300 MiniMiner: use FeeFrac in AncestorFeerateComparator (glozow)
Pull request description:
Closes#30284. Closes#30367, see https://github.com/bitcoin/bitcoin/issues/30367#issuecomment-2217459257
Previously, we were only comparing feerates up to 1/1000 precision, since CFeeRate comparison just looks at their respective nSatoshisPerK. This could lead to MiniMiner selecting packages in the wrong order (i.e. by txid) if their feerates were less than 0.001sat/vB different. Fix this by creating + comparing `FeeFrac`s instead.
Also, `FeeFrac::Mul` doesn't have the overflow problem.
Also added a few minor fuzzer fixups that caught my eye while I was debugging this.
ACKs for top commit:
ismaelsadeeq:
Tested ACK 09370529fb
murchandamus:
ACK 09370529fb with nits
dergoegge:
tACK 09370529fb
Tree-SHA512: e5b6d6c3f7289f30cd8280d0a47cd852d0180b83d1b27ff9514f50c97103b0f069484e48cba2ca3a57419beadc1996c1b9dd8d0a0f34bc4f4223d8adaf414ce5
The script building logic performs a quadratic number of copies in the
number of nested wrappers in the miniscript. Limit the number of nested
wrappers to avoid fuzz timeouts.
Thanks to Marco Falke for reporting the fuzz timeouts and providing a
minimal input to reproduce.
This target may call into logic quadratic over the number of
sub-fragments. Limit the number of sub-fragments to keep the runtime
reasonable.
Thanks to Marco Falke for reporting the fuzz timeouts with a minimized
input.
This avoids situations during a reindex in which shutdown
doesn't finish since SyncWithValidationInterfaceQueue is
called by the load block thread when the scheduler is already stopped.
fa360b047f util: Use SteadyClock in RandAddSeedPerfmon (MarcoFalke)
Pull request description:
`GetTime` is mockable in tests and system-changeable in production. This should be fine and not lead to issues, but using `SteadyClock` is more correct in this context to do an expensive task only so often.
ACKs for top commit:
sipa:
utACK fa360b047f
TheCharlatan:
ACK fa360b047f
Tree-SHA512: 1958b9e9e356c9801ac981014b4b528cfc8ce6612853d8b45f6519b16f0b1839ff765abb8b3368b86f00958ddc6a686f6b90278c57a7ad4858bdf3ea33775cca
26a7f70b5d ci: enable self-assignment clang-tidy check (Cory Fields)
32b1d13792 refactor: add self-assign checks to classes which violate the clang-tidy check (Cory Fields)
Pull request description:
See comment here: https://github.com/bitcoin/bitcoin/pull/30161#issuecomment-2148229582
Our code failed these checks in three places, which have been fixed up here. Though these appear to have been harmless, adding the check avoids the copy in the self-assignment case so there should be no downside.
~Additionally, minisketch failed the check as well. See https://github.com/sipa/minisketch/pull/87~
Edit: Done
After fixing up the violations, turn on the aggressive clang-tidy check.
Note for reviewers: `git diff -w` makes this trivial to review.
ACKs for top commit:
hebasto:
ACK 26a7f70b5d, I have reviewed the code and it looks OK.
TheCharlatan:
ACK 26a7f70b5d
Tree-SHA512: 74d8236a1b5a698f2f61c4740c4fc77788b7f882c4b395acc4e6bfef1ec8a4554ea8821a26b14d70cfa6c8e2e9ea305deeea3fbf323967fa19343c007a53c5ba
3333bae9b2 tidy: modernize-use-equals-default (MarcoFalke)
Pull request description:
Prior to C++20, `modernize-use-equals-default` could have been problematic because it could turn a non-aggregate into an aggregate. The risk would be that aggregate initialization would be enabled where the author did not intend to enable it.
With C++20, aggregate for those is forbidden either way. (https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1008r1.pdf)
So enabled it for code clarity, consistency, and possibly unlocking compiler optimizations. See https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-default.html
ACKs for top commit:
stickies-v:
ACK 3333bae9b2
Tree-SHA512: ab42ff01be7ca7e7d8b4c6a485e68426f59627d83dd827cf292304829562348dc17a52ee009f5f6f3c1c2081d7166ffac4baef23197ebeba8de7767c6ddfe255
fa601ab9f7 util: Catch translation string errors at compile time (MarcoFalke)
Pull request description:
The translation helper function `_()` has many problems. For example, the following compiles:
```cpp
auto ptr{"wrong"};
_(ptr);
_(nullptr);
_(0);
_(NULL);
```
However, it is wrong, because none of the arguments passed to the function can be picked up by the translation tooling for transifex.
Fix all issues by enforcing only real string literals can be passed to the function.
ACKs for top commit:
ryanofsky:
Code review ACK fa601ab9f7
hebasto:
ACK fa601ab9f7.
Tree-SHA512: 33aed02d7e8fc9bfb8f90746f5c8072a8c0910fa900ec3516af2e732780b0fee8b07b6596c0fc210b018c0869111d6c34bf8d083de0e88ecdb4dee88e809186d
e233ec036d refactor: Use designated initializer (Hodlinator)
Pull request description:
Block was recently touched (e2d1f84858) and the codebase recently switched to C++20 which allows this to improve robustness.
Follow-up suggested in https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1664818014
ACKs for top commit:
maflcko:
ACK e233ec036d
Tree-SHA512: ce3a18f513421e923710a43c8f97db1badb7ff5c6bdbfd62d9543312d2225731db5c14bef16feb47c43b84fad4dc24485086634b680feba422d2b7b363e13fa6
If the snapshot is not an ancestor of the most-work header (m_best_header),
syncing from that alternative chain should be prioritised.
Therefore don't accept loading a snapshot in this situation.
If that other chain turns out to be invalid, m_best_header
would be reset and loading the snapshot should be possible again.
Because of the work required to generate a conflicting headers chain,
this should only be possible under extreme circumstances, such as major forks.
e8c3b7172c remove truc_policy.cpp from libbitcoin_common_a_SOURCES (glozow)
Pull request description:
Hebasto pointed out that it doesn't need to be there since it's in `libbitcoin_node_a_SOURCES`
ACKs for top commit:
maflcko:
ACK e8c3b7172c
hebasto:
ACK e8c3b7172c, this change follows the design [docs](https://github.com/bitcoin/bitcoin/blob/master/doc/design/libraries.md).
ismaelsadeeq:
ACK e8c3b7172c
Tree-SHA512: ebe6b0dda2d097d88c37d2b071ac99da3e9c519ec473d4b8f870a50f1b24d00e2e5deef317fb0f6a91c96103e7f37468cb8f13395818eab55a42af48df4e0fc6
8ce3739edb test: verify wallet is still active post-migration failure (furszy)
771bc60f13 wallet: Use LegacyDataSPKM when loading (Ava Chow)
61d872f1b3 wallet: Move MigrateToDescriptor and DeleteRecords to LegacyDataSPKM (Ava Chow)
b231f4d556 wallet: Move LegacyScriptPubKeyMan::IsMine to LegacyDataSPKM (Ava Chow)
7461d0c006 wallet: Move LegacySPKM data storage and handling to LegacyDataSPKM (Ava Chow)
517e204bac Change MigrateLegacyToDescriptor to reopen wallet as BERKELEY_RO (Ava Chow)
Pull request description:
#26606 introduced `BerkeleyRODatabase` which is an independent parser for BDB files. This PR uses this in legacy wallet migration so that migration will continue to work once the legacy wallet and BDB are removed. `LegacyDataSPKM` is introduced to have the minimum data and functions necessary for a legacy wallet to be loaded for migration.
ACKs for top commit:
cbergqvist:
ACK 8ce3739edb
theStack:
Code-review ACK 8ce3739edb
furszy:
Code review ACK 8ce3739edb
Tree-SHA512: dccea12d6c597de15e3e42f97ab483cfd069e103611200279a177e021e8e9c4e74387c4f45d2e58b3a1e7e2bdb32a1d2d2060b1f8086c03eeaa0c68579d9d54e
