This extra method will be needed for updating the filter set for
faster wallet rescans; after an internal top-up has happened, we only
want to add the newly created scriptPubKeys.
This is useful for speeding up wallet rescans and is based on an
earlier version from PR #15845 ("wallet: Fast rescan with BIP157 block
filters"), which was never merged.
Co-authored-by: MacroFake <falke.marco@gmail.com>
393be86724 build, msvc: Drop no longer required macro definitions for leveldb (Hennadii Stepanov)
Pull request description:
Since levedb v1.21:
- the `__STDC_LIMIT_MACROS` macro definition is unneeded;
commit: [50fbc87e8c62a816d6afd4740e0652a13ac6dc3e](50fbc87e8c)
- the `LEVELDB_ATOMIC_PRESENT` macro is unused;
commit: [04f39105c5a418905da8b7657ca244d672c99d3b](04f39105c5)
ACKs for top commit:
sipsorcery:
tACK 393be86724.
jarolrod:
crACK 393be86724
Tree-SHA512: 845bb310474cf17f9f0f70a585054a222914313a0fc28c42a023133250efbb9ab7fb2d1ebb355943233b6fd24d5bddfb73fbbf5dce71ea232ae652af8b695149
When a block is initially read from a blk*.dat file during reindexing,
it can be added to the block index only if all of its ancestor blocks
have been added, which is rare. If the block's ancestors have not been
added, the block must be re-read from disk later when it can be added.
This commit: During the initial block read, deserialize only its header,
rather than the entire block, since this is sufficient to determine
if its parent (and thus all its ancestors) has been added. This is a
performance improvement.
SkipTo() reads data from the file into the CBufferedFile object
(memory), but, unlike this object's read() method, SkipTo() doesn't
transfer data into a caller's memory buffer. This is useful because
after skipping forward in the stream in this way, the user can, if
needed, rewind the stream (SetPos()) and access the object's memory
buffer including ranges that were skipped over (without needing to
read from the disk file).
e1eadaa72d Revert "test: check importing wallets when blocks are pruned throw an error" (Aurèle Oulès)
Pull request description:
The test doesn't pass (not detected by the normal CI, because it is an extended test):
```
Traceback (most recent call last):
File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/test_framework/test_framework.py", line 133, in main
self.run_test()
File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/feature_pruning.py", line 480, in run_test
self.wallet_test()
File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/feature_pruning.py", line 361, in wallet_test
assert_raises_rpc_error(-4, "Importing wallets is disabled when blocks are pruned", self.nodes[2].importwallet, "abc")
File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/test_framework/util.py", line 130, in assert_raises_rpc_error
assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/test_framework/util.py", line 145, in try_rpc
raise AssertionError(
AssertionError: Expected substring not found in error message:
substring: 'Importing wallets is disabled when blocks are pruned'
error message: 'Only legacy wallets are supported by this command'.
```
So revert it for now, which will be done anyway in https://github.com/bitcoin/bitcoin/pull/24865/commits. (This commit is taken from there)
ACKs for top commit:
andrewtoth:
ACK e1eadaa72d
Tree-SHA512: 10f556ea1aa97dc9cb64f91055977eecb9534f658170aabb4909c3e85ca6c20588c7a021219356fab678e0e2bec999d347facd00054f07a9445ad393e6353b4c
7ad15d1100 [net processing] Handle IsContinuationOfLowWorkHeadersSync return value correctly when new headers sync is started (dergoegge)
Pull request description:
This PR fixes a bug in the headers sync logic that enables submitting headers to a nodes block index that don't lead to a chain that surpasses our DoS limit.
The issue is that we ignore the return value on [the first `IsContinuationOfLowWorkHeadersSync` call after a new headers sync is started](fabc031048/src/net_processing.cpp (L2553-L2568)), which leads to us passing headers to [`ProcessNewBlockHeaders`](fabc031048/src/net_processing.cpp (L2856)) when that initial `IsContinuationOfLowWorkHeadersSync` call returns `false`. One easy way (maybe the only?) to trigger this is by sending 2000 headers where the last header has a different `nBits` value than the prior headers (which fails the pre-sync logic [here](fabc031048/src/headerssync.cpp (L189))). Those 2000 headers will be passed to `ProcessNewBlockHeaders`.
I haven't included a test here so far because we can't test this without changing the default value for `CRegTestParams::consensus.fPowAllowMinDifficultyBlocks` or doing some more involved refactoring.
ACKs for top commit:
sipa:
ACK 7ad15d1100
glozow:
ACK 7ad15d1100
Tree-SHA512: 9aabb8bf3700401e79863d0accda0befd2a83c4d469a53f97d827e51139e2f826aee08cdfbc8866b311b153f61fdac9b7aa515fcfa2a21c5e2812c2bf3c03664
a079103c94 gui: update peers window "Transaction Relay" label and tooltip (Jon Atack)
Pull request description:
to current v24.0 p2p behavior. Similar updates have been made to RPC getpeerinfo and CLI -netinfo.
Top commit has no ACKs.
Tree-SHA512: 400a794f655f799eefcb77c479fef6bcd3f81aede2af54a4a9bcb7c0c783e2e3f18bc5fd2484a79e8c30af279747a05fc0ebb69dbc47375d4c55b16ceba97b99
c2a21c0670 gui: use fallback value for Version and User Agent during peer connection (Jon Atack)
Pull request description:
During connection setup for a peer, getpeerinfo returns `"version": 0, "subver": ""` and the GUI Peers window displays 0 and an empty field, respectively.
Give these fields the same behavior as the other fields in the GUI Peers window: display the fallback value in `src/qt/forms/debugwindow.ui` (i.e. `N/A`) until a valid result is available after the peer connection completes.
An alternative would be to display nothing for both, as is the case currently for User Agent.
ACKs for top commit:
jarolrod:
ACK c2a21c0670
furszy:
code ACK c2a21c06
Tree-SHA512: 4f0060fa9abde120a2bb48c9dcc87894d9bb70c33e6ab43b22400a4bcd0ceff0fa098adf7f385b0a7a4cf5d7053463b36fe1232e19a8d5025eecd8db9833f73b
1184a66347 doc: Rearrange some lines in the dependency graph of libraries (Stacie Waleyko)
Pull request description:
In this PR, I've attempted to improve readability in the [dependency graph of libraries](https://github.com/bitcoin/bitcoin/blob/master/doc/design/libraries.md) by untangling a few crossed lines. I'm not sure if this is that big of an improvement but wanted to throw it out there.
I used an extremely scientific method of manually counting the number of crossed lines in the original diagram and got 15. This PR reduces that number down to about 10.
I also changed the curve of the lines to "basis" which rounds the edges out. Again, not sure if it really is that much of an improvement, but it seems marginally easier on the eyes.
Here is what the new graph looks like rendered:
The changes can be verified independently with [Mermaid](https://mermaid-js.github.io/mermaid/#/), with the easiest way being the online editor: https://mermaid.live/
I did try moving some more stuff around, particularly the top level of library callers, but was not able to simplify the graph any further.
ACKs for top commit:
shaavan:
ACK 1184a66347
Tree-SHA512: 61d33d68c1e6fa354aebdda5e06e9c7a722ca20886c6acc30dd08af7133d737130d7a646d87f9e5a8ae0bc5a5aabfbc64ded9ee04dfeed8f23d948444add916b
fa51cc9651 refactor: Use type-safe time point for CWallet::m_next_resend (MacroFake)
Pull request description:
`GetTime` is not type-safe, thus deprecated, see 75cbbfa279/src/util/time.h (L62-L70)
ACKs for top commit:
shaavan:
Code Review ACK fa51cc9651
aureleoules:
ACK fa51cc9651
Tree-SHA512: 030de10070518580763ea75079442e2f934c54d3083be3ebe35e7f1bc6db2096745bb46d95aa1e6efe29ced30a048acfe5cd999178e6787b7647dfbec5ecb444
Since levedb v1.21:
- the `__STDC_LIMIT_MACROS` macro definition is unneeded;
commit: 50fbc87e8c62a816d6afd4740e0652a13ac6dc3e
- the `LEVELDB_ATOMIC_PRESENT` macro is unused;
commit: 04f39105c5a418905da8b7657ca244d672c99d3b
ff138f9cf1 doc: add `scanblocks` to list of descriptor RPCs (Sebastian Falbesoner)
Pull request description:
This is a tiny documentation follow-up to #23549.
ACKs for top commit:
aureleoules:
ACK ff138f9cf1
shaavan:
ACK ff138f9cf1
Tree-SHA512: cc45b78c13ec4aa5bac688648f8e83f04a9ae54376e67371b280428f0253e2585cf0575fa51e3835d4c13c940559bfcdd88d7504bf97a81b2a73bb34a0db7577
dddd1acf58 net: Set relay in version msg to peers with relay permission (MacroFake)
Pull request description:
Seems odd to set the `relay` permission in -blocksonly mode and also ask the peer not to relay transactions.
ACKs for top commit:
dergoegge:
ACK dddd1acf58
naumenkogs:
ACK dddd1acf58
mzumsande:
ACK dddd1acf58
Tree-SHA512: 7bb0e964993ea4982747ae2801fe963ff88586e2ded03015b60ab83172b5b61f2d50e9cde9d7711b7ab207f8639467ecafc4d011ea151ec6c82c722f510f4df7
deba6fe315 test: update feature_config_args.py (josibake)
2e3826cbcd util: warn if reindex is used in conf (josibake)
5e744f4238 util: disallow setting conf in bitcoin.conf (josibake)
Pull request description:
In help from `bitcoind -h` it specifes that `conf` can only be used from the commandline. However, if `conf` is set in a `bitcoin.conf` file, there is no error and from reading the logs it seems as if the `conf=<other file>` is being used, despite it being ignored. To recreate, you can setup a `bitcoin.conf` file in the default directory, add `conf=<some other file>.conf` and in the separate config file set whichever config value you want and verify that it is being ignored. alternatively, if you set `includeconf=<some other file>.conf` , your config in `<some other file>` will be picked up.
This PR fixes this by having the node error when reading the config file if `conf=` is set.
Additionally, it was mentioned in a recent [PR review club](https://bitcoincore.reviews/24858) that if `reindex=1` is set in the config file, the node will reindex on every startup, which is undesirable:
```irc
17:14 <larryruane> michaelfolkson: Reindex is requested by the user (node operator) as a configuration option (command line or in the config file, tho you probably would never put it in the file, or else it would reindex on every startup!)
```
This PR also has a commit to warn if `reindex=1` is set in the config file.
ACKs for top commit:
hebasto:
ACK deba6fe315, tested on Ubuntu 22.04.
aureleoules:
tACK deba6fe315
ryanofsky:
Code review ACK deba6fe315.
Tree-SHA512: 619fd0aa14e98af1166d6beb92651f5ba3f10d38b8ee132957f094f19c3a37313d9f4d7be2e4019f3fc9a2ca5fa42d03eb539ad820e27efec7ee58a26eb520b1
315fd4dbab test: Test for out of bounds vout in sendall (Andrew Chow)
b132c85650 wallet: Check utxo prevout index out of bounds in sendall (Andrew Chow)
708b72b715 test: Test that sendall works with watchonly spending specific utxos (Andrew Chow)
6bcd7e2a3b wallet: Correctly check ismine for sendall (Andrew Chow)
Pull request description:
The `sendall` RPC would previously fail when used with a watchonly wallet and specified inputs. This failure was caused by checking isminetype equality with ISMINE_ALL rather than a bitwise AND as IsMine can never return ISMINE_ALL.
Also added a test.
ACKs for top commit:
w0xlt:
ACK 315fd4dbab
furszy:
ACK 315fd4db
Tree-SHA512: fb55cf6524e789964770b803f401027319f0351433ea084ffa7c5e6f1797567a608c956b7f7c5bd542aa172c4b7b38b07d0976f5ec587569efead27266e8664c
3e9d0bea8d build: only run high priority benchmarks in 'make check' (furszy)
466b54bd4a bench: surround main() execution with try/catch (furszy)
3da7cd2a76 bench: explicitly make all current benchmarks "high" priority (furszy)
05b8c76232 bench: add "priority level" to the benchmark framework (furszy)
f1593780b8 bench: place benchmark implementation inside benchmark namespace (furszy)
Pull request description:
This is from today's meeting, a simple "priority level" for the benchmark framework.
Will allow us to run certain benchmarks while skip non-prioritized ones in `make check`.
By default, `bench_bitcoin` will run all the benchmarks. `make check`will only run the high priority ones,
and have marked all the existent benchmarks as "high priority" to retain the current behavior.
Could test it by modifying any benchmark priority to something different from "high", and
run `bench_bitcoin -priority-level=high` and/or `bench_bitcoin -priority-level=medium,low`
(the first command will skip the modified bench while the second one will include it).
Note: the second commit could be avoided by having a default arg value for the priority
level but.. an explicit set in every `BENCHMARK` macro call makes it less error-prone.
ACKs for top commit:
kouloumos:
re-ACK 3e9d0bea8d
achow101:
ACK 3e9d0bea8d
theStack:
re-ACK 3e9d0bea8d
stickies-v:
re-ACK 3e9d0bea8d
Tree-SHA512: ece59bf424c5fc1db335f84caa507476fb8ad8c6151880f1f8289562e17023aae5b5e7de03e8cbba6337bf09215f9be331e9ef51c791c43bce43f7446813b054
e133264c5b Add test for PSBT input verification (Greg Sanders)
d25699280a Verify PSBT inputs rather than check for fields being empty (Greg Sanders)
Pull request description:
In a few keys spots, PSBT finality is checked by looking for non-empty witness data.
This complicates a couple things:
1) Empty data can be valid in certain cases
2) User may be passed bogus final data by a counterparty during PSBT work happening, and end up with incorrect signatures that they may not be able to check in other contexts if the UTXO doesn't exist yet in chain/mempool, timelocks, etc.
On the whole I think these heavier checks are worth it in case someone is actually assuming the signatures are correct if our API is saying so.
ACKs for top commit:
achow101:
ACK e133264c5b
Tree-SHA512: 9de4fbb0be1257b081781f5df908fd55666e3acd5c4e36beb3b3f2f5a6aed69ff77068c44cde6127e159e773293fd9ced4c0bb47e693969f337e74dc8af030da
5d3f98d278 refactor: Replace m_params with chainman.GetParams() (Aurèle Oulès)
Pull request description:
Fixes a TODO introduced in #24595.
Removes `m_params` from `CChainState` class and replaces it with `m_chainman.GetParams()`.
ACKs for top commit:
MarcoFalke:
review ACK 5d3f98d278🌎
Tree-SHA512: de0fe31450d281cc7307c0d820495e86c93c7998e77a148db2c703da66cff1059e6560c041f1864913c42075aa24d259c2623d45e929ca0a8056ed330a9f9978
1c48dae76f test: Use C++11 member initializers for TestMemPoolEntryHelper (MacroFake)
fad7f2239c test: Remove unused txmempool include from tests (MacroFake)
Pull request description:
Seems odd to include this heavy header in all tests despite it only being used in a few tests.
Can be reviewed with `--color-moved=dimmed-zebra --ignore-all-space`
ACKs for top commit:
aureleoules:
reACK 1c48dae76f
hebasto:
ACK 1c48dae76f, I have reviewed the code and it looks OK, I agree it can be merged.
w0xlt:
ACK 1c48dae76f
Tree-SHA512: 31f2808d04ec33bfc2409832b8e59e6c870eaa98fbcf879e1c786492c7d07134711b30f8290bdb34e1b8f7b8f2f11dae8e10c64e7eb31f584b2f5c58fcc7743b
b147322a7a Use `PACKAGE_NAME` in messages rather than hardcoding "Bitcoin Core" (Hennadii Stepanov)
Pull request description:
Usually, we do not hardcode "Bitcoin Core" in the user-faced messages.
See:
- bitcoin/bitcoin#18646
- bitcoin/bitcoin#19282
Also grammar has been improved -- singular instead of plural.
ACKs for top commit:
jarolrod:
ACK b147322a7a
Tree-SHA512: b135c18703dfdd7b63d4cb27d1ac48f6a9dbf69382142ae381f33bf561cbf57477a11d1c73263aa834f705206d7dd5716df2523d38ed0d4cfec8babc38bb017a
