016cc807f7 test: wallet migration, add coverage for tx extra data (furszy)
Pull request description:
Quick follow-up to #28610, coming from https://github.com/bitcoin/bitcoin/pull/28610#pullrequestreview-1802823938.
Verifying that the 'replaced_by_txid' and 'replaces_txid' tx data is preserved after migration,
as well as the extra tx comments.
ACKs for top commit:
jamesob:
Nice, ACK 016cc807f7
achow101:
ACK 016cc807f7
pablomartin4btc:
ACK 016cc807f7
BrandonOdiwuor:
lgtm ACK 016cc807f7
Tree-SHA512: 697cabece730cbe5c5947bf98455e80a8877c0352fbe2a66362ce5ea530b67882b0bec561a67d48fee200cdad717cd62c57fd809e2a94ff83c3fad30021e1d9e
e60fc7d5d3 logging: Replace uses of LogPrintfCategory (Anthony Towns)
f7ce5ac08c logging: add LogError, LogWarning, LogInfo, LogDebug, LogTrace (Anthony Towns)
fbd7642c8e logging: add -loglevelalways=1 option (Anthony Towns)
782bb6a056 logging: treat BCLog::ALL like BCLog::NONE (Anthony Towns)
667ce3e329 logging: Drop BCLog::Level::None (Anthony Towns)
ab34dc6012 logging: Log Info messages unconditionally (Anthony Towns)
dfe98b6874 logging: make [cat:debug] and [info] implicit (Anthony Towns)
c5c76dc615 logging: refactor: pull prefix code out (Anthony Towns)
Pull request description:
Replace `LogPrint*` functions with severity based logging functions:
* `LogInfo(...)`, `LogWarning(...)`, `LogError(...)` for unconditional (uncategorised) logging (replaces `LogPrintf`)
* `LogDebug(CATEGORY, ...)` and `LogTrace(CATEGORY, ...)` for conditional logging (replaces `LogPrint`)
* `LogPrintLevel(CATEGORY, LEVEL, ...)` for when the level isn't known in advance, or a category needs to be added for an info/warning/error log message (mostly unchanged, but rarely needed)
Logs look roughly as they do now with `LogInfo` not having an `[info]` prefix, and `LogDebug` having a `[cat]` prefix, rather than a `[cat:debug]` prefix. This removes `BCLog::Level::None` entirely -- for `LogFlags::NONE` just use `Level::Info`, for any actual category, use `Level::Debug`.
Adds docs to developer-notes about when to use which level.
Adds `-loglevelalways=1` option so that you get `[net:debug]`, `[all:info]`, `[all:warning]` etc, which might be helpful for automated parsing, or just if you like everything to be consistent. Defaults to off to reduce noise in the default config, and to avoid unnecessary changes on upgrades.
Changes the behaviour of `LogPrintLevel(CATEGORY, BCLog::Level::Info, ...)` to be logged unconditionally, rather than only being an additional optional logging level in addition to trace and debug. Does not change the behaviour of `LogPrintLevel(NONE, Debug, ...)` and `LogPrintLevel(NONE, Trace, ...)` being no-ops.
ACKs for top commit:
maflcko:
re-ACK e60fc7d5d3🌚
achow101:
ACK e60fc7d5d3
stickies-v:
ACK e60fc7d5d3
jamesob:
ACK e60fc7d5d3 ([`jamesob/ackr/28318.1.ajtowns.logging_simplify_api_for`](https://github.com/jamesob/bitcoin/tree/ackr/28318.1.ajtowns.logging_simplify_api_for))
Tree-SHA512: e7a4588779b148242495b7b6f64198a00c314cd57100affab11c43e9d39c9bbf85118ee2002792087fdcffdea08c84576e20844b3079f27083e26ddd7ca15d7f
931575418e test: assumeutxo: spend coin from snapshot chainstate after loading (Sebastian Falbesoner)
Pull request description:
This PR extends the AssumeUTXO functional test by submitting a spending transaction for an UTXO that is only available in a the snapshot chainstate (after loading via `loadtxoutset`), i.e. it hasn't been seen in a block before. With that we can verify that snapshot coins are visible to the mempool.
Note that we unfortunately can't use MiniWallet here, as the only available UTXO to spend from the snapshot chainstate is at height 200, where a P2PKH created from the test framework's deterministic private key is used (see `TestNode.generate(...)` and the `PRIV_KEYS` array). Coinbase outputs with smaller heights (<= 199) would be part of the pre-generated chain and hence not qualify for the "UTXO is only in snapshot chainstate and has never been seen in a block" scenario, coinbase outputs with larger heights (>= 201) can't be spent due to immaturity, as the snapshot chainstate block height is 299.
One could of course mine a different chain with outputs that MiniWallet supports (e.g. taproot anyone-can-spend), but this would change the hardcoded AssumeUTXO hash, colliding with other PRs like #28838, so I wanted to avoid that.
ACKs for top commit:
maflcko:
lgtm ACK 931575418e
jamesob:
ACK 931575418e
Tree-SHA512: 0665868e1e91fe74f408d0a239cc264bbbc11a6b55bcc0e86cc8b4b2ec1f44977884b817dbe9065a7c768332cab464636656858bc8b9c8e7d7810498e0a17d78
fb5bfed26a cli: add transport protcol column to -netinfo (Martin Zumsande)
9eed22e870 net: attempt v2 transport for addrfetch connections if we support it (Martin Zumsande)
770c0311ef net: attempt v2 transport for manual connections if we support it (Martin Zumsande)
Pull request description:
Some preparations before enabling `-v2transport` as the default:
* Use v2 for `-connect`, `-addnode` config arg and `-seednode` if `-v2transport` is enabled.
Our peer may or may not support v2, but I don't think an extra option is necessary for any of these (we have that for the `addnode` rpc), because we have the reconnection mechanism that will try again with `v1` if our peer doesn't support `v2`.
* Add a column for the transport protocol to `-netinfo`. I added it next to the `net` column because I thought it looked nice there, but if people prefer it somewhere else I'm happy to move it.
ACKs for top commit:
sipa:
utACK fb5bfed26a
achow101:
ACK fb5bfed26a
stratospher:
tested ACK fb5bfed. addrfetch + manual connections aren't frequent and it would be useful to have this for transition to v2 one day.
theStack:
ACK fb5bfed26a
kristapsk:
ACK fb5bfed26a
Tree-SHA512: c4575ad11b99613870b342acae369fa08f877ac79e6e04eb62e94ad7a92d528e289183c0963c78aa779ba11cb91e2a6fad7c8b0d813126c46c3e5b54bd962c26
9d728916b2 net: create I2P sessions with both ECIES-X25519 and ElGamal encryption (Jon Atack)
Pull request description:
A Bitcoin Core node may only connect to a peer destination via I2P if both sides have sessions with the same encryption type. Encryption type is a property of the session, not the destination. Sessions may support multiple encryption types.
As Bitcoin Core is not currently setting the encryption type when creating I2P sessions, it uses the older default, ElGamal (type 0).
This pull updates our I2P session creation to use both ECIES-X25519 and ElGamal (types 4 and 0, respectively). This allows to connect to I2P peers of either type, and the newer, faster ECIES-X25519 will be preferred.
See also:
- discussion around https://github.com/qbittorrent/qBittorrent/issues/19625#issuecomment-1879582395
- recently updated "Signature and Encryption Types" in https://geti2p.net/en/docs/api/samv3
Thank you and credit to zzzi2p for reporting and to vort for the patch.
Closes https://github.com/bitcoin/bitcoin/issues/29197.
ACKs for top commit:
zzzi2p:
ACK 9d728916b2
recursive-rat4:
ACK 9d728916b2
kristapsk:
cr utACK 9d728916b2
brunoerg:
crACK 9d728916b2
shaavan:
crACK 9d728916b2
Tree-SHA512: 0912fc01af9706914a7854f7479b9d82fc86c9530466cad8674e30f7eb4894d90d514efbc1aee8b7ea690faa6ff4a23b62cf5de8737cffdbc463300082c9b917
080763a058 build: Drop `ALLOW_HOST_PACKAGES` support in depends (Hennadii Stepanov)
Pull request description:
The `ALLOW_HOST_PACKAGES` variable was introduced in bitcoin#10508 "to speed up build and avoid timeout".
It is no longer the case for our CI infrastructure, which uses self- hosted persistent workers and depends caching.
In the current circumstances, it does not seem worth porting this feature to the upcoming [CMake-based](https://github.com/bitcoin/bitcoin/issues/28607) build system.
ACKs for top commit:
fanquake:
ACK 080763a058 - I can't imagine this option got any use outside our CI. It's also mostly just at odds with the idea of a self-contained dependency builder.
TheCharlatan:
ACK 080763a058
Tree-SHA512: 36f52690be913479c5d12be36760b8de1a6e891fe7c2cf98a7b8d6561006a6b18631e431351d79e97edb9409f9902d032aedf7b963aa7615e54b59fc2a58f7d6
e5b9ee0221 fuzz: set `nMaxOutboundLimit` in connman target (brunoerg)
Pull request description:
Setting `nMaxOutboundLimit` (`-maxuploadtarget`) will make fuzz to reach more coverage in connman target. This value is used in `GetMaxOutboundTimeLeftInCycle`, `OutboundTargetReached` and `GetOutboundTargetBytesLeft`.
ACKs for top commit:
dergoegge:
utACK e5b9ee0221
jonatack:
ACK e5b9ee0221
Tree-SHA512: d19c83602b0a487e6da0e3be539aa2abc95b8bbf36cf9a3e391a4af53b959f68ca38548a96d27d56742e3b772f648da04e2bf8973dfc0ab1cdabf4f2e8d44de6
The `ALLOW_HOST_PACKAGES` variable was introduced in bitcoin#10508 "to
speed up build and avoid timeout".
It is no longer the case for our CI infrastructure, which uses self-
hosted persistent workers and depends caching.
In the current circumstances, it does not seem worth porting this
feature to the upcoming CMake-based build system.
406b71abcb wallet: Migrate entire address book entries (Andrew Chow)
Pull request description:
Not all of the data in an address book entry was being copied to the watchonly and solvables wallets. This includes information such as whether the address was previously spent, and any receive requests that may exist. A test has been added to check that the previously spent information is copied, although it passes without the changes in this PR since this information is also regenerated when a transaction is loaded/added into a wallet.
ACKs for top commit:
ryanofsky:
Code review ACK 406b71abcb. Just suggested change since last review
furszy:
Code review ACK 406b71ab
Tree-SHA512: 13de42b16a1d8524fe0555764744139566b2e7d29741ceffc1158a905dd537136b762330568b3b5cac28cbee1bfd363a20de97d0a6c5296738cb3aa99133945b
5779010ed7 RPC/Blockchain: scanblocks: Accept named param for filter_false_positives (Luke Dashjr)
Pull request description:
Possibly due to a silent cross-merge, `scanblocks` was left out of 96233146dd
ACKs for top commit:
stickies-v:
ACK 5779010ed7
theStack:
ACK 5779010ed7
Tree-SHA512: bade107c7cb5fdd1265224c263a1e1edfc8bc0698b3abfac8d65c49a270181f0311713f7243813de17932a7a7ca65a36850e527ab0b433cf64c32191d3adde70
A Bitcoin Core node may only connect to a peer destination via I2P if both sides
have sessions with the same encryption type. The encryption type is a property
of the session, not the destination. Sessions may support multiple encryption
types.
As Bitcoin Core is not currently setting the I2P encryption type when creating
sessions, it is using the older default, ElGamal (type 0).
This pull updates Bitcoin Core to use both ECIES-X25519 and ElGamal (types 4 and
0, respectively). This allows to connect to I2P peers with either type, and the
newer, faster ECIES-X25519 will be preferred.
See also the recently updated section "Signature and Encryption Types" in
https://geti2p.net/en/docs/api/samv3
Thanks and credit to zzzi2p (https://github.com/zzzi2p) for reporting.
Closes https://github.com/bitcoin/bitcoin/issues/29197.
d83bea42d1 wallettool: Don't create CWallet when dumping DB (Andrew Chow)
40c80e36b1 wallettool: Don't unilaterally reset wallet_instance if loading error (Ava Chow)
Pull request description:
https://github.com/bitcoin/bitcoin/issues/29109#issuecomment-1863449058 reports that a wallet with noncritical errors cannot be dumped with `bitcoin-wallet dump`. This was caused by an erroneous reset of the wallet pointer when the loading the wallet returns something other than `LOAD_OK`. Not all errors are errors that require aborting, so unilaterally resetting the pointer at that time is incorrect. The first commit resolves this issue.
Furthermore, if a wallet has loading errors, that should not prevent the wallet tool from dumping the wallet. The wallet application logic should not get in the way of performing such a low level database operation, especially when it's primary usage is for debugging potentially corrupted wallets. The 2nd commit is taken from #28710 and changes the `dump` to stop at making a `WalletDatabase` rather than making a `CWallet` only to retrieve the underlying `WalletDatabase`.
ACKs for top commit:
furszy:
Code review ACK d83bea42d1
BrandonOdiwuor:
Code Review ACK d83bea42d1
Tree-SHA512: 425d712dfff1002bd81272aca0bae1016f9126a3c89506f8cb7cf0a0ec9f33d0c03b8d03896394f3a45c2998e59047e19218dfd08dc8a5f40e8625134e886b0f
fa87f8feb7 doc: Clarify C++20 comments (MarcoFalke)
Pull request description:
Turns out "class template argument deduction for aggregates" is one of the few things implemented only in recent compilers, see https://en.cppreference.com/w/cpp/compiler_support/20
So clarify the comments.
ACKs for top commit:
hebasto:
ACK fa87f8feb7, I verified the code with clang-{16,17}.
Tree-SHA512: f6d20f946cb6f8e34db224e074ed8f9dfa598377c066d1b58a8feb9e64d007444f1e2c0399e91a3e282fd5d59f90e0d7df90aa3956824d96bc78070ee12f603c
f8ca1357c8 build: Fix check whether `-latomic` needed (Hennadii Stepanov)
Pull request description:
Clang >=15 still might need linking against `libatomic`.
We use `std::atomic<std::chrono::seconds>::compare_exchange_strong` in `net_processing.cpp`.
Addresses the https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1440293694.
ACKs for top commit:
maflcko:
lgtm ACK f8ca1357c8
fanquake:
ACK f8ca1357c8
Tree-SHA512: ba8b6a88fd3471a206d068e8a000a053c99cb46d26bd04624418ddb066b3b9664a569ec8a1569af67c96b3e27f13dccbd5e24f985290ac072b6d74c92524e35d
fa46cc22bc Remove deprecated -rpcserialversion (MarcoFalke)
Pull request description:
The flag is problematic for many reasons:
* It is deprecated
* It is a global flag, requiring a restart to change, as opposed to a flag that can be set on each RPC invocation
* It may be hidden in config files by accident, hard to debug, causing LND crashes and bugs, see https://github.com/bitcoin/bitcoin/issues/28730#issuecomment-1780940868
* It makes performance improvements harder to implement: https://github.com/bitcoin/bitcoin/pull/17529#issuecomment-556082818
Fix all issues by removing it.
If there is a use-case, likely a per-RPC flag can be added, if needed.
ACKs for top commit:
ajtowns:
crACK fa46cc22bc
TheCharlatan:
lgtm ACK fa46cc22bc
Tree-SHA512: 96ba1c60356ce93954fe5c2a59045771c6d1516ad0d9dc436ef1800a1f1b0153f0d5fb78ca99d53ad54ba25fbce36962bdf1d4325aceedfc8154a61347a6a915
a44808fb43 fuzz: rule-out too deep derivation paths in descriptor parsing targets (Antoine Poinsot)
Pull request description:
This fixes the `mocked_descriptor_parse` timeout reported in #28812 and direct the targets more toward what they are intended to fuzz: the descriptor syntax.
ACKs for top commit:
sipa:
utACK a44808fb43
achow101:
ACK a44808fb43
dergoegge:
ACK a44808fb43 - Not running into timeouts anymore
TheCharlatan:
ACK a44808fb43
Tree-SHA512: a5dd1dbe9adf8f088bdc435addab88b56f435e6d7d2065bd6d5c6d80a32e3f1f97d3d2323131ab233618cd6dcc477c458abe3c4c865ab569449b8bc176231e93
29fde0223a Squashed 'src/secp256k1/' changes from 199d27cea3..efe85c70a2 (fanquake)
Pull request description:
This includes changes from the 0.4.1 release: https://github.com/bitcoin-core/secp256k1/releases/tag/v0.4.1.
> The point multiplication algorithm used for ECDH operations (module ecdh) was replaced with a slightly faster one.
> Optional handwritten x86_64 assembly for field operations was removed because modern C compilers are able to output more efficient assembly. This change results in a significant speedup of some library functions when handwritten x86_64 assembly is enabled (--with-asm=x86_64 in GNU Autotools, -DSECP256K1_ASM=x86_64 in CMake), which is the default on x86_64. Benchmarks with GCC 10.5.0 show a 10% speedup for secp256k1_ecdsa_verify and secp256k1_schnorrsig_verify.
ACKs for top commit:
hebasto:
re-ACK e2cdeb5925
jonasnick:
reACK e2cdeb5925
Tree-SHA512: eaa82721b63e84b9d8dae82956d5e75dbcee50c58c9049b7901055d79aef938bd268e18ce4ff85feb73aae7ee1cf58018b93067692f8f69f80216d336bd6f10a
faebf1df2a wallet: Fix use-after-free in WalletBatch::EraseRecords (MarcoFalke)
Pull request description:
Creating a copy of the pointer to the underlying data of the stream is not enough to copy the data.
Currently this happens to work sometimes, because the stream may not immediately free unused memory. However, there is no guarantee by the stream interface to always behave this way. Also, if `vector::clear` is called on the underlying memory, any pointers to it are invalid.
Fix this, by creating a full copy of all bytes.
ACKs for top commit:
achow101:
ACK faebf1df2a
Tree-SHA512: 79ede9bc16cf257609545597bc6d9623ceead4531780ea6037cc5684aa3a7c7d80601354d315358defe47193f978a8ce40c5dc4637e32936c76157679b549ac5
b1318dcc56 test: change `m_submitted_in_package` input to fuzz data provider boolean (ismaelsadeeq)
5615e16b70 tx fees: update `m_from_disconnected_block` to `m_mempool_limit_bypassed` (ismaelsadeeq)
fcd4296648 doc: fix typo and update incorrect comment (ismaelsadeeq)
562664d263 test: wait for fee estimator to catch up before estimating fees (ismaelsadeeq)
Pull request description:
This is a simple PR that does two things
1. Fixes#29000 by waiting for the fee estimator to catch up after `removeForBlock` calls before calling `estimateFee` in the `BlockPolicyEstimates` unit test.
2. Addressed some outstanding review comments from #28368
- Updated `NewMempoolTransactionInfo::m_from_disconnected_block` to `NewMempoolTransactionInfo::m_mempool_limit_bypassed` which now correctly indicates what the boolean does.
- Changed input of `processTransaction`'s tx_info `m_submitted_in_package` input from false to fuzz data provider boolean.
- Fixed some typos, and update incorrect comment
ACKs for top commit:
martinus:
re-ACK b1318dcc56
glozow:
utACK b1318dcc56
Tree-SHA512: 45268729bc044da4748fe004524e0df696d2ec92c5bd053db9aad6e15675f3838429b2a7b9061a6b694be4dc319d1782a876b44df506ddd439d62ad07252d0e1
e03d6f7ed5 fuzz: set `m_fallback_fee`/`m_fee_mode` in `wallet_fees` target (brunoerg)
Pull request description:
`m_fallback_fee` and `m_fee_mode` are used in `GetMinimumFeeRate` but we're not setting any value for them in `wallet_fees` target. That's the reason fuzzing is never reaching the following code:
This PR fixes it.
ACKs for top commit:
maflcko:
review ACK e03d6f7ed5
achow101:
ACK e03d6f7ed5
murchandamus:
ACK e03d6f7ed5
Tree-SHA512: 5d364f5351d65762a3ddf88e3abb7bda401b7e4955285e083031d216fb50082b1ea98e2c065aff75a5a8a3d1bc4c2e5e3ca9f9478d902ee8f8d4347b6cbe53af
fa1d49542e refactor: share and use `GenerateRandomKey` helper (Sebastian Falbesoner)
Pull request description:
Making the `GeneratingRandomKey` helper (recently introduced in PR #28433, commit b6934fd03f) available to other modules via key.{h.cpp} allows us to create random private keys directly at CKey instantiation, in contrast to the currently needed two-step process of creating an (invalid) CKey instance first and then having to call `MakeNewKey(...)`.
This is mostly used in unit tests and a few instances in the wallet.
ACKs for top commit:
Sjors:
re-ACK fa1d49542e
achow101:
ACK fa1d49542e
sipa:
utACK fa1d49542e
kristapsk:
cr utACK fa1d49542e
stratospher:
ACK fa1d495.
Tree-SHA512: 6fec73f33efe5bd77ca7d3c2fc06725d96f789f229294c39377e682ff222cfc7990b77c92e0bfd4cb6cf891d007ab1f86d395907511f06e87044bae37652a2fd
In reality some mempool transaction might be submitted in a package,
so change m_submitted_in_package to fuzz data provider boolean just like
m_has_no_mempool_parents.
The boolean indicates whether the transaction was added without enforcing mempool
fee limits. m_mempool_limit_bypassed is the correct variable name.
Also changes NewMempoolTransactionInfo booleans descriptions to the format that
is consistent with the codebase.
This affects manual connections made either with -connect, or with
-addnode provided as a bitcoind config arg (the addnode RPC has an
extra option for v2).
We don't necessarily know if our peer supports v2, but will reconnect
with v1 if they don't. In order to do that, improve the reconnection
behavior such that we will reconnect after a sleep of 500ms
(which usually should be enough for our peer to send us their
version message).
Making the `GenerateRandomKey` helper available to other modules via
key.{h.cpp} allows us to create random private keys directly at
instantiation of CKey, in contrast to the two-step process of creating
the instance and then having to call `MakeNewKey(...)`.
