97181decf5 Add test for negative transaction version w/ CSV to tx_valid.json (Chris Stewart)
Pull request description:
This PR adds a static test vector corresponding to the bug found in various implementations of the bitcoin protocol discovered by dergoegge
For more information see:
https://delvingbitcoin.org/t/disclosure-btcd-consensus-bugs-due-to-usage-of-signed-transaction-version/455
ACKs for top commit:
darosior:
ACK 97181decf5
dergoegge:
ACK 97181decf5
Tree-SHA512: 92bbcd3cd10a569757b4de91e1b2bcfebc2b75ddb0160be36d8e512a6fa4623cced1aba93bd1cc044962cd2b10e1d184ef109ccdfe3cfcf85cf4b9585d80d115
This test-only RPC is required when a TestNode initiates
an outbound v2 p2p connection. Add a new arg `v2transport`
so that the node can attempt v2 connections.
Introduces functionality to detect when limited peers connections
are desirable or not. Ensuring that the new connections desirable
services flags stay relevant throughout the software's lifecycle.
(Unlike the previous approach, where once the validation IBD flag
was set, the desirable services flags remained constant forever).
This will let us recover from stalling scenarios where the node had
successfully synced, but subsequently dropped connections and remained
inactive for a duration longer than the limited peers threshold (the
timeframe within which limited peers can provide blocks). Then, upon
reconnection to the network, the node may end up only establishing
connections with limited peers, leading to an inability to synchronize
the chain.
This also fixes a possible limited peers threshold violation during IBD,
when the user configures `-maxtipage` further than the BIP159's limits.
This rule violation could lead to sync delays and, in the worst-case
scenario, trigger the same post-IBD stalling scenario (mentioned above)
but during IBD.
In the process of doing so, refactor `ConsumeNetAddr()` to generate the
addresses from IPv4, IPv6, Tor, I2P and CJDNS networks in the same way -
by preparing some random stream and deserializing from it. Similar code
was already found in `RandAddr()`.
Hopefully, refraining users from modifying the file unless they are
certain about the potential consequences.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
The preceding "Unable to parse settings file" message lacked
the necessary detail and guidance for users on what steps to
take next in order to resolve the startup error.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
282b12ddb0 refactor: remove CTxMemPool::queryHashes() (stickies-v)
Pull request description:
`CTxMemPool::queryHashes()` is only used in `MempoolToJSON()`, where it can just as easily be replaced with the more general `CTxMemPool::entryAll()`. No behaviour change, just cleans up the code.
ACKs for top commit:
dergoegge:
Code review ACK 282b12ddb0
TheCharlatan:
ACK 282b12ddb0
glozow:
ACK 282b12ddb0. Looks like there's no conflicts.
Tree-SHA512: 16160dec8e1f2457fa0f62dc96d2d2efd92c4bab810ecdb0e08918b8e85a667702c8e41421eeb4ea6abe92a5956a2a39a7a6368514973b78be0d22de2ad299b2
Instead turning tx.vout into a vector of `CRecipient`, make `FundTransaction`
take a `CRecipient` vector directly. This allows us to remove SFFO logic from
the wrapper RPC `FundTransaction` since the `CRecipient` objects have already
been created with the correct SFFO values. This also allows us to remove
SFFO from both `FundTransaction` function signatures.
This sets us up in a future PR to be able to use these RPCs with BIP352
static payment codes.
Move validation logic out of `CreateRecipients` and instead take the
already validated outputs from `ParseOutputs` as an input.
Move SFFO parsing out of `CreateRecipients` into a new function,
`InterpretSubtractFeeFromOutputsInstructions`. This takes the SFFO instructions
from `sendmany` and `sendtoaddress` and turns them into a set of integers.
In a later commit, we will also move the SFFO parsing logic from
`FundTransaction` into this function.
Worth noting: a user can pass duplicate addresses and addresses that dont exist
in the transaction outputs as SFFO args to `sendmany` and `sendtoaddress`
without triggering a warning. This behavior is preserved in to keep this commit
strictly a refactor.
Move the parsing and validation out of `AddOutputs` into its own function,
`ParseOutputs`. This allows us to re-use this logic in `ParseRecipients` in a
later commit, where the code is currently duplicated.
The new `ParseOutputs` function returns a CTxDestination,CAmount tuples.
This allows the caller to then translate the validated outputs into
either CRecipients or CTxOuts.
Move the univalue formatting logic out of AddOutputs and into its own function,
`NormalizeOutputs`. This allows us to re-use this logic in later commits.
5555d8db33 test: Use blocks_path where possible (MarcoFalke)
fa9108941f rpc: Fix race in loadtxoutset (MarcoFalke)
Pull request description:
The tip may have advanced, also if it did not, there is no reason to
have two variables point to the same block.
Fixes https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1344694600
ACKs for top commit:
achow101:
ACK 5555d8db33
pablomartin4btc:
ACK 5555d8db33
BrandonOdiwuor:
Code Review ACK 5555d8db33
Tree-SHA512: 23a82924a915b61bb1adab8ad20ec8914139c8ee647817af34ca27ee310a2e45833d8b285503e0feebe63e4667193d6d98cfcbbc1509bf40712225e04dd19e8b
`CWallet::GetEncryptionKey()` would return a reference to the internal
`CWallet::vMasterKey`, guarded by `CWallet::cs_wallet`, which is unsafe.
Returning a copy would be a shorter solution, but could have security
implications of the master key remaining somewhere in the memory even
after `CWallet::Lock()` (the current calls to
`CWallet::GetEncryptionKey()` are safe, but that is not future proof).
So, instead of `EncryptSecret(m_storage.GetEncryptionKey(), ...)`
change the `GetEncryptionKey()` method to provide the encryption
key to a given callback:
`m_storage.WithEncryptionKey([](const CKeyingMaterial& k) { EncryptSecret(k, ...); })`
This silences the following (clang 18):
```
wallet/wallet.cpp:3520:12: error: returning variable 'vMasterKey' by reference requires holding mutex 'cs_wallet' [-Werror,-Wthread-safety-reference-return]
3520 | return vMasterKey;
| ^
```
6044628543 crypto, hash: replace custom rotl32 with std::rotl (Fabian Jahr)
Pull request description:
While exploring some C++20 changes and checking against our code I found this potential improvement:
1. We can replace our custom implementation of `rotl32` in crypto/chacha20 with `std::rotl` from the [new `bit` header](https://en.cppreference.com/w/cpp/header/bit).
ACKs for top commit:
fanquake:
ACK 6044628543
Tree-SHA512: db55b366f20fca2ef62e5f10a838f8a709d531678c35c1dba20898754029c788a2fd47995208ed6d187cf814109a7ca397bc2c301504500aee79da04c95d6895
fa96d93711 refactor: Allow std::span construction from CKey (MarcoFalke)
999962d68d Add missing XOnlyPubKey::data() to get mutable data (MarcoFalke)
Pull request description:
Is is possible to construct a `Span` from a reference to a `CKey`. However, the same is not possible with `std::span`.
Fix that.
ACKs for top commit:
shaavan:
ReACK fa96d93711
willcl-ark:
ACK fa96d93711
Tree-SHA512: 44fccdce5f32bc16b44f3b1bd32e86d9eabfd09bca6abe79f2d6db0cb0b5e4aaeaff710f023cb21ccde9315d2007d55f1b43f29416e81bceeeabe3948f673d3a
3ba815b42d Make v2transport default for addnode RPC when enabled (Pieter Wuille)
Pull request description:
Since #29058, several types of manually configured connections will attempt v2 connections when `-v2transport` is enabled, except for the `addnode` RPC, as that one has an explicit argument to enable or disable.
Make the default for that RPC match the `-v2transport` setting so the behavior matches that of other manual connections from a user perspective.
ACKs for top commit:
achow101:
ACK 3ba815b42d
kristapsk:
ACK 3ba815b42d
theStack:
Code-review ACK 3ba815b42d
Tree-SHA512: 31ef48cf1e533abb17866020378c004df929e626074dc98b3229fb60a66de58435e95c8fda8d1b463e1208aa39d1f42d239818e7e58595a3738089920598befc
cdc6ac4126 snapshots: don't core dump when running -checkblockindex after `loadtxoutset` (Mark Friedenbach)
Pull request description:
Transaction counts aren't known for block history loaded from a snapshot. If you start with `-checkblockindex` after loading a snapshot, the bitcoin daemon will core dump. The test suite does not check for this because all the snapshots have no non-coinbase transactions (all blocks prior to the snapshot are assumed to have `nTx = 1`).
Recommend for backport to 26.x
ACKs for top commit:
fjahr:
utACK cdc6ac4126
achow101:
ACK cdc6ac4126
pablomartin4btc:
tACK cdc6ac4126
Tree-SHA512: f7488a85cc29056e2ac443ce8f34aea4dfde6ba246efce82235d6a4dca2dca4344f07b93c93424b4addcb83e4cb2ae49a3ebb37d89840d42d2aeea35904cab04
74ebd4d135 doc, test: Test and explain service flag handling (Martin Zumsande)
Pull request description:
Service flags received from the peer-to-peer network are handled differently, depending on how we receive them.
If received directly from an outbound peer the flags belong to, they replace existing flags.
If received via gossip relay (so that anyone could send them), new flags are added, but existing ones but cannot be overwritten.
Document that and add test coverage for it.
ACKs for top commit:
achow101:
ACK 74ebd4d135
furszy:
ACK 74ebd4d135
brunoerg:
utACK 74ebd4d135
Tree-SHA512: 604adc3304b8e3cb1a10dfd017025c10b029bebd3ef533f96bcb5856fee5d4396a9aed4949908b8e7ef267ad21320d1814dd80f88426330c5c9c2c529c497591
It's preferable to use type-safe transaction identifiers to avoid
confusing txid and wtxid. The next commit will add a reference to this
set; we use this opportunity to change it to Txid ahead of time instead
of adding new uses of uint256.
ec779a2b8e doc: add unconditional info loglevel following merge of PR 28318 (Jon Atack)
Pull request description:
Commit ab34dc6012 of #28318 was an incomplete version of [`118c756` (#25203)](118c7567f6) from the `Severity-based logging` parent PR.
Add the missing text to update the `-loglevel` help doc.
While here, make the help text a little easier to understand.
Can be tested by running:
```
./src/bitcoind -regtest -help-debug | grep -A12 loglevel=
```
before
```
-loglevel=<level>|<category>:<level>
Set the global or per-category severity level for logging categories
enabled with the -debug configuration option or the logging RPC:
info, debug, trace (default=debug); warning and error levels are
always logged.
```
after
```
-loglevel=<level>|<category>:<level>
Set the global or per-category severity level for logging categories
enabled with the -debug configuration option or the logging RPC.
Possible values are info, debug, trace (default=debug). The
following levels are always logged: error, warning, info.
```
ACKs for top commit:
stickies-v:
ACK ec779a2b8e
Tree-SHA512: 0c375e30a5a4c168ca7d97720e8c287f598216767afedae329824e09a480830faf8537b792c5c4bb647c68681c287fe3005c62093708ce85624e9a71c8245e42
This avoids cases of missing -O2, when *FLAGS has been overriden.
Removes the need for duplicate code to clear autoconf defaults.
Also, move CORE_CXXFLAGS before DEBUG_CXXFLAGS, so that -O2 is always
overriden if debugging etc.
2d1b1c7dae build: remove --enable-lto (fanquake)
Pull request description:
This has outlived its usefulness, doesn't gel well with newer compilers & `-flto` related options, i.e thin vs full, or `=auto`, and having `-flto` as the only option means that sometimes this just needs to be worked around, i.e in oss-fuzz:
https://github.com/google/oss-fuzz/blob/master/projects/bitcoin-core/build.sh.
While it was convenient when `-flto` was newer, support for `-flto` is now in all compilers we use, and there's also no-longer any real need for us to treat `-flto` different to any other optimization option.
Remove it, to remove build complexity, and so there's no need to port a similar option to CMake.
Note that the LTO option remains in depends, because we still a way to build packages that have LTO specific patches/options.
ACKs for top commit:
TheCharlatan:
ACK 2d1b1c7dae
hebasto:
ACK 2d1b1c7dae.
Tree-SHA512: 91812de7da35346f51850714a188fcffbac478bc8b348bf756c2555fcbde86ba622ac2fb77d294dea0378c741d3656f06121ef3a795aeed63fd170fc31bfa5af
eb78ea4eeb [log] mempool loading (glozow)
Pull request description:
Motivated by #29193. Currently, we only log something (non-debug) when we fail to load the file and at the end of importing all the transactions. That means it's hard to tell what's happening if it's taking a long time to load.
This PR adds a maximum of 10 new unconditional log lines:
- When we start to load transactions.
- Our progress percentage when it advances by at least 10% from the last time we logged. Percentage is based on the number of transactions.
If there are lots of transactions in the mempool, the logs will look like this:
```
2024-01-11T11:36:30.410726Z Loading 401 mempool transactions from disk...
2024-01-11T11:36:30.423374Z Progress loading mempool transactions from disk: 10% (tried 41, 360 remaining)
2024-01-11T11:36:30.435539Z Progress loading mempool transactions from disk: 20% (tried 81, 320 remaining)
2024-01-11T11:36:30.447874Z Progress loading mempool transactions from disk: 30% (tried 121, 280 remaining)
2024-01-11T11:36:30.460474Z Progress loading mempool transactions from disk: 40% (tried 161, 240 remaining)
2024-01-11T11:36:30.473731Z Progress loading mempool transactions from disk: 50% (tried 201, 200 remaining)
2024-01-11T11:36:30.487806Z Progress loading mempool transactions from disk: 60% (tried 241, 160 remaining)
2024-01-11T11:36:30.501739Z Progress loading mempool transactions from disk: 70% (tried 281, 120 remaining)
2024-01-11T11:36:30.516334Z Progress loading mempool transactions from disk: 80% (tried 321, 80 remaining)
2024-01-11T11:36:30.531309Z Progress loading mempool transactions from disk: 90% (tried 361, 40 remaining)
2024-01-11T11:36:30.549019Z Imported mempool transactions from disk: 401 succeeded, 0 failed, 0 expired, 0 already there, 400 waiting for initial broadcast
```
If there are 0 or 1 transactions, progress logs aren't printed.
ACKs for top commit:
kevkevinpal:
Concept ACK [eb78ea4](eb78ea4eeb)
ismaelsadeeq:
ACK eb78ea4eeb
dergoegge:
Code review ACK eb78ea4eeb
theStack:
re-ACK eb78ea4eeb
mzumsande:
tested ACK eb78ea4eeb
Tree-SHA512: ae4420986dc7bd5cb675a7ebc76b24c8ee60007f0296ed37e272f1c3415764d44963bea84c51948da319a65661dca8a95eac2a59bf7e745519b6fcafa09812cf
No behavior change. Just an intermediate refactoring.
By relocating the peer desirable services flags into the peer
manager, we allow the connections acceptance process to handle
post-IBD potential stalling scenarios.
In the follow-up commit(s), the desirable service flags will be
dynamically adjusted to detect post-IBD stalling scenarios (such
as a +48-hour inactive node that must prefer full node connections
instead of limited peer connections because they cannot provide
historical blocks). Additionally, this encapsulation enable us
to customize the connections decision-making process based on
new user's configurations in the future.
And implement 'ApproximateBestBlockDepth()' to estimate
the distance, in blocks, between the best-known block
and the network chain tip. Utilizing the best-block time
and the chainparams blocks spacing to approximate it.
AttachChain will create the chain notifications handler which contains a
reference to the wallet's shared_ptr. If AttachChain fails, the wallet
needs to be unloaded, and this is expected to happen with its custom
deleter ReleaseWallet. However, if the chain notifications handler is
still set, then the shared_ptr is still referenced by something, so the
wallet is never actually released.
0eebd6fe7d test: Assert that a new tx with a delta of 0 is never added (kevkevin)
cfdbcd19b3 rpc: exposing modified_fee in getprioritisedtransactions (kevkevin)
252a86729a rpc: renaming txid -> transactionid (kevkevin)
2fca6c2dd0 rpc: changed prioritisation-map -> "" (kevkevin)
3a118e19e1 test: Directly constructing 2 entry map for getprioritisedtransactions (kevkevin)
Pull request description:
In this PR I am addressing some comments in https://github.com/bitcoin/bitcoin/pull/27501 as a followup.
- changed `prioritisation-map` in the `RPCResult` to `""`
- Directly constructing 2 entry map for getprioritisedtransactions in functional tests
- renamed `txid` to `transactionid` in `RPCResult` to be more consistent with naming elsewhere
- exposed the `modified_fee` field instead of having it be a useless arg
- Created a new test that asserts when `prioritisedtransaction` is called with a fee_delta of 0 it is not added to mempool
ACKs for top commit:
glozow:
reACK 0eebd6fe7d, only change is the doc suggestion
Tree-SHA512: e99056e37a8b1cfc511d87c83edba7c928b50d2cd6c2fd7c038976779850677ad37fddeb2b983e8bc007ca8567eb21ebb78d7eae9b773657c2b297299993ec05
aaaace2fd1 fuzz: Assume presence of __builtin_*_overflow, without checks (MarcoFalke)
fa223ba5eb Revert "build: Fix undefined reference to __mulodi4" (MarcoFalke)
fa7c751bd9 build: Bump clang minimum supported version to 14 (MarcoFalke)
Pull request description:
Most supported operating systems ship with clang-14 (or later), so bump the minimum to that and allow new code to drop workarounds for previous clang bugs.
For reference:
* https://packages.debian.org/bookworm/clang (`clang-14`)
* https://packages.ubuntu.com/jammy/clang (`clang-14`)
* CentOS-like 8/9 Stream: All Clang versions from 15 to 17
* FreeBSD 12/13: All Clang versions from 15 to 16
* OpenSuse Tumbleweed ships with https://software.opensuse.org/package/clang (`clang17`); No idea about OpenSuse Leap
On operating systems where the clang version is not shipped by default, the user would have to use GCC, or install clang in a different way. For example:
* https://packages.debian.org/bullseye/g++ (g++-10)
* https://packages.ubuntu.com/focal/g++-10
* https://apt.llvm.org/, or nix, or guix, or compile clang from source, ...
ACKs for top commit:
fanquake:
ACK aaaace2fd1
Tree-SHA512: 81d066b14cc568d27312f1cc814b09540b038a10a0a8e9d71fc9745b024fb6c32a959af673e6819b817ea7cef98da4abfa63dff16cffb7821b40083016b0291f
