fa76f0d0efccd1ea272a46060022eea3e998268e refactor: Make m_count_with_* in CTxMemPoolEntry int64_t, drop UBSAN supp (MarcoFalke)
Pull request description:
This is a refactor as long as no signed integer overflow appears. In normal operation and absent bugs, signed integer overflow should never happen in the touched code paths.
The main benefit of this refactor is to drop the file-wide ubsan suppression `unsigned-integer-overflow:txmempool.cpp`.
For now, this only changes the internal private representation and the publicly returned type remains `uint64_t`.
ACKs for top commit:
glozow:
ACK fa76f0d0ef
ryanofsky:
Code review ACK fa76f0d0efccd1ea272a46060022eea3e998268e
Tree-SHA512: a09e33a915d60c65d369d44ba1a45ce4a6a76e6dc2bea43216ba02b5eab0b74e214b2c7cc44360493f2c483d18d96e4636b7a75b23050976efc80e38de852c39
a1e653828bc59351b2a0dd5a70f519e6b61199bc test: Add test for migrating default wallet and plain file wallet (Andrew Chow)
bdbe3fd76b4b9186503dc1926a2fa3f8178d00a5 wallet: Generated migrated wallet's path from walletdir and name (Andrew Chow)
Pull request description:
This PR fixes an assertion error that is hit during the setup of the new database during migration of a wallet that was not contained in a wallet dir. Also added a test for this case as well as one for migrating the default wallet.
ACKs for top commit:
ryanofsky:
Code review ACK a1e653828bc59351b2a0dd5a70f519e6b61199bc
furszy:
ACK a1e65382
Tree-SHA512: 96b218c0de8567d8650ec96e1bf58b0f8ca4c4726f5efc6362453979b56b9d569baea0bb09befb3a5aed8d16d29bf75ed5cd8ffc432bbd4cbcad3ac5574bc479
clang <=17 warns on -nostdlibinc, which causes an error on our -Werror builds.
Note that this breaks the "-fPIE" check in configure because it relies on
catching warnings, but that is not a problem for macOS.
daa5a658c0e79172e4dea0758246f11281790d29 refactor: rename BCLog::BLOCKSTORE to BLOCKSTORAGE (Jon Atack)
cf622b214bfe0a97e403f1e9dc54bf5bbfc59fc3 doc: release note re raising on invalid -debug/debugexclude/loglevel (Jon Atack)
6cb1c66041ee14dbedad3aeeb90190ea5dddf917 init: remove config option names from translated -loglevel strings (Jon Atack)
25478292726dd7208b22a8924c8f1fdeac5c33f5 test: -loglevel raises on invalid values (Jon Atack)
a9c295888b82c86ef4629aa2d9061ea152b48f20 init: raise on invalid loglevel config option (Jon Atack)
b0c3995393c592fa96306e077ed64e65d5400882 test: -debug and -debugexclude raise on invalid values (Jon Atack)
4c3c19d943a0a4cf191495f6ebe9b964835607a4 init: raise on invalid debug/debugexclude config options (Jon Atack)
Pull request description:
and rename BCLog::BLOCKSTORE to BLOCKSTORAGE so the enum is the same as its value like the other BCLog enums.
Per discussion in bitcoin-core-dev IRC today from https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2023-05-11#921458.
ACKs for top commit:
achow101:
ACK daa5a658c0e79172e4dea0758246f11281790d29
ryanofsky:
Code review ACK daa5a658c0e79172e4dea0758246f11281790d29. Just translated string template cleanup since last review
pinheadmz:
re-ACK daa5a658c0e79172e4dea0758246f11281790d29
Tree-SHA512: 4c107a93d8e8ce4e2ee81d44aec672526ca354ec390b241221067f68204beac8b4ba7a65748bcfa124ff2245c4307fa9243ec4fe0b464d0fa69c787fb322c3cc
a72af2e833bcf7baa79a246609da90ae0ade2a82 bench: disable birth time block skip for wallet_create_tx.cpp (furszy)
Pull request description:
As the benchmarks inside `wallet_create_tx.cpp` assert the wallet
balance at the end, they require all blocks to be scanned by the wallet.
So, we need to ensure that no blocks are skipped by the recently added
wallet birth time functionality.
This just means setting the wallet birth time to the genesis block time.
So the wallet is always older than any new block.
ACKs for top commit:
achow101:
ACK a72af2e833bcf7baa79a246609da90ae0ade2a82
hernanmarino:
ACK a72af2e833bcf7baa79a246609da90ae0ade2a82
TheCharlatan:
ACK a72af2e833bcf7baa79a246609da90ae0ade2a82
Tree-SHA512: d3148659bd633d20978736e1292e3456a2c6dd2b6c8f60625a4160e16818d923487c889237eb3f34693f7dd78b7d124b89afdc56e4c9fad370026d0733ef1e08
faa05d1965b03d997c1814447d7772f3d43bcbdb fuzz: Fix implicit-integer-sign-change in wallet/fees fuzz target (MarcoFalke)
Pull request description:
This fixes a bug in the fuzz target.
```
echo 'OiAAAPr//wAAAAAAAAA=' | base64 --decode > /tmp/a
UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" FUZZ=wallet_fees ./src/test/fuzz/fuzz /tmp/a
```
```
wallet/fees.cpp:58:58: runtime error: implicit conversion from type 'unsigned int' of value 4294574080 (32-bit, unsigned) to type 'int' changed the value to -393216 (32-bit, signed)
#0 0x5625ef46a094 in wallet::GetMinimumFeeRate(wallet::CWallet const&, wallet::CCoinControl const&, FeeCalculation*) src/wallet/fees.cpp:58:58
#1 0x5625eedd467f in wallet::(anonymous namespace)::wallet_fees_fuzz_target(Span<unsigned char const>) src/wallet/test/fuzz/fees.cpp:64:11
...
SUMMARY: UndefinedBehaviorSanitizer: implicit-integer-sign-change wallet/fees.cpp:58:58 in
ACKs for top commit:
dergoegge:
tACK faa05d1965b03d997c1814447d7772f3d43bcbdb
brunoerg:
ACK faa05d1965b03d997c1814447d7772f3d43bcbdb
Tree-SHA512: 66a4020d6a4153a92c7023e9f94ec6279862566db7236ce3cf6951b7fbee616dc88a56fe9502de4099d74f9840439b20a984b0733fb432e43129e774bcc2a6e6
d2b39e09bc6a5982fc5cf4b538b7fdb0e3cae576 test: ensure old fee_estimate.dat not read on restart and flushed (ismaelsadeeq)
cf219f29f3c5b41070eaab9a549a476f01990f3a tx fees, policy: read stale fee estimates with a regtest-only option (ismaelsadeeq)
3eb241a141defa564c94cb95c5bbaf4c5bd9682e tx fees, policy: do not read estimates of old fee_estimates.dat (ismaelsadeeq)
5b886f2b436eaa8c2b7de58dc4644dc6223040da tx fees, policy: periodically flush fee estimates to fee_estimates.dat (ismaelsadeeq)
Pull request description:
Fixes#27555
The issue arises when an old `fee_estimates.dat` file is sometimes read during initialization.
Or after an unclean shutdown, the latest fee estimates are not flushed to `fee_estimates.dat`.
If the fee estimates in the old file are old, they can cause transactions to become stuck in the mempool.
This PR ensures that nodes do not use stale estimates from the old file during initialization. If `fee_estimates.dat`
has not been updated for 60 hours or more, it is considered stale and will not be read during initialization. To avoid
having old estimates, the `fee_estimates.dat` file will be flushed periodically every hour. As mentioned #27555
> "The immediate improvement would be to store fee estimates to disk once an hour or so to reduce the chance of having an old file. From there, this case could probably be detected, and refuse to serve estimates until we sync."
In addition, I will follow-up PR to persist the `mempoolminfee` across restarts.
ACKs for top commit:
willcl-ark:
ACK d2b39e09bc
instagibbs:
reACK d2b39e09bc
glozow:
ACK d2b39e09bc6a5982fc5cf4b538b7fdb0e3cae576. One nit if you follow up.
Tree-SHA512: 4f6e0c296995d0eea5cf80c6aefdd79b7295a6a0ba446f2166f32afc105fe4f831cfda1ad3abd13c5c752b4fbea982cf4b97eaeda2af1fd7184670d41edcfeec
Any identifier starting with two _, or one _ followed by a capital letter is reserved for the compiler and thus must not be used. See: https://stackoverflow.com/a/228797/7130273
-BEGIN VERIFY SCRIPT-
s() { git grep -l "$1" src | xargs sed -i "s/$1/$2/g"; }
s '__pushKV' 'pushKVEnd'
s '_EraseTx' 'EraseTxNoLock'
s '_Other' 'Other'
-END VERIFY SCRIPT-
40b333e21f8741e2f553df6b5dcff7277c00a982 fuzz: wallet, add target for CoinControl (Ayush Singh)
Pull request description:
This PR adds fuzz coverage for `wallet/coincontrol`.
Motivation: Issue [#27272](https://github.com/bitcoin/bitcoin/issues/27272#issue-1628327906)
The idea is to create different/unique instances of `COutPoint` by placing it inside the `CallOneOf` function, which may or may not be consumed by all of the `CoinControl` file's methods.
This is my first PR on Bitcoin Core, and I will try my best to address any reviews/changes ASAP. I'm also working on fuzz harness files for other files in the wallet and plan to open PR for them soon.
ACKs for top commit:
kevkevinpal:
reACK [40b333e](40b333e21f)
MarcoFalke:
lgtm ACK 40b333e21f8741e2f553df6b5dcff7277c00a982
achow101:
ACK 40b333e21f8741e2f553df6b5dcff7277c00a982
brunoerg:
crACK 40b333e21f8741e2f553df6b5dcff7277c00a982
dergoegge:
ACK 40b333e21f8741e2f553df6b5dcff7277c00a982
Tree-SHA512: 174769f4e86df8590b532b85480fd620082587e84e50e49ca9b52f0588a219355362cefd66250dd9942e86019d27af4ca599b45e871e9f147d2cc0ba97c4aa7b
In functional tests it is a quite common scenario to generate fresh
elliptic curve keypairs, which is currently a bit cumbersome as it
involves multiple steps, e.g.:
privkey = ECKey()
privkey.generate()
privkey_wif = bytes_to_wif(privkey.get_bytes())
pubkey = privkey.get_pubkey().get_bytes()
Simplify this by providing a new `generate_keypair` helper function that
returns the private key either as `ECKey` object or as WIF-string
(depending on the boolean `wif` parameter) and the public key as
byte-string; these formats are what we mostly need (currently we don't
use `ECPubKey` objects from generated keypairs anywhere).
With this, most of the affected code blocks following the pattern above
can be replaced by one-liners, e.g.:
privkey, pubkey = generate_keypair(wif=True)
Note that after this commit, the only direct uses of `ECKey` remain in
situations where we want to set the private key explicitly, e.g. in
MiniWallet (test/functional/test_framework/wallet.py) or the test for
the signet miner script (test/functional/tool_signet_miner.py).
Exceptions are not propagated in ctype callback functions used by bcc.
This means an AssertionError exception raised by check_equal() to signal
a failed assertion is not getting caught and properly logged. Instead,
the error is logged to stdout and execution of the handler stops.
The current workaround to check whether all check_equal() assertions in
a callback succeeded is to increment a success counter after the
assertions (which only gets incremented if none exception is raised and
stops execution). Then, outside the callback, the success counter can be
used to check whether a callback executed successfully.
One issue with the described workaround is that when an exception
occurs, there is no way of telling which of the check_equal() statements
caused the exception; moreover, there is no way of inspecting how the
pieces of data that got compared in check_equal() differed (often
a crucial clue when debugging what went wrong).
Two fixes to this problem come to mind. The first involves having the
callback function make event data accessible outside the callback and
inspecting the event using check_equal() outside the callback. This
solution still requires a counter in the callback to tell whether
a callback was actually executed or if instead the call to
perf_buffer_poll() timed out.
The second fix entails wrapping all relevant check_equal() statements
inside callback functions into try-catch blocks and manually logging
AssertionErrors. While not as elegant in terms of design, this approach
can be more pragmatic for more complex tests (e.g., ones involving
multiple events, events of different types, or the order of events).
The solution proposed here is to select the most pragmatic fix on
a case-by-case basis: Tests in interface_usdt_net.py,
interface_usdt_mempool.py and interface_usdt_validation.py have been
refactored to use the first approach, while the second approach was
chosen for interface_usdt_utxocache.py (partly to provide a reference
for the second approach, but mainly because the utxocache tests are the
most intricate tests, and refactoring them to use the first approach
would negatively impact their readability). Lastly,
interface_usdt_coinselection.py was kept unchanged because it does not
use check_equal() statements inside callback functions.
14405e8d4d259c18a21fc006d0a27550be3171f8 doc: test: update TestShell instructions (ismaelsadeeq)
Pull request description:
Fixes #27904
From #27904 and IRC.
Update [Testshell instructions ](https://github.com/bitcoin/bitcoin/blob/master/test/functional/test-shell.md#2-importing-testshell-from-the-bitcoin-core-repository)
E.g `TestShell.setup()` throws
```
AttributeError: type object 'TestShell' has no attribute 'setup'
```
Parentheses are missing, it should be `TestShell().setup()`
ACKs for top commit:
Sjors:
utACK 14405e8d4d259c18a21fc006d0a27550be3171f8
brunoerg:
crACK 14405e8d4d259c18a21fc006d0a27550be3171f8
hernanmarino:
utACK 14405e8d4d259c18a21fc006d0a27550be3171f8
Tree-SHA512: ffe5fa1103a3b00ef0ee99879adae967b0da07cb8f8451c4c261b0a70b3b666af7aeaacd6f46f85a84ee5e9c7c7ed49700209b5b1f124d7a76efc420ad5c9cd9
As the benchmarks inside wallet_create_tx.cpp assert the
wallet balance at the end, they require all
blocks to be scanned by the wallet. So, we need
to ensure that no blocks are skipped by the recently
added wallet birth time functionality.
This just means setting the wallet birthtime to the
genesis block time. So the wallet is always older than
any new block.
5524fa00faebfe040f126a4152640f9e9ed572b1 doc: add release note about removal of `deprecatedrpc=walletwarningfield` flag (Sebastian Falbesoner)
5c77db73542fe4c76fd53526ae560d56dde5f830 Restorewallet/createwallet help documentation fixups/improvements (Jon Atack)
a00ae31fccba63d5fd409ffb39c1622df2ea3e8c rpc: remove deprecated "warning" field from {create,load,restore,unload}wallet (Sebastian Falbesoner)
Pull request description:
The "warning" string field for wallet creating/loading RPCs (`createwallet`, `loadwallet`, `unloadwallet` and `restorewallet`) has been deprecated with the configuration option `-deprecatedrpc=walletwarningfield` in PR #27279 (released in v25.0). For the next release v26.0, the field and the configuration option can be removed.
ACKs for top commit:
achow101:
ACK 5524fa00faebfe040f126a4152640f9e9ed572b1
jonatack:
ACK 5524fa00faebfe040f126a4152640f9e9ed572b1
Tree-SHA512: 8212f72067d08095304018b8a95d2ebef630004b65123483fbbfb078cc5709c2d825bbc35b16ea5f6b28ae7377347382d7e9afaf7bdbf0575d2c229d970784de
fc6c17b83887ef193f2b97264b1843c94dcb915d build: make sure we can overwrite config.{guess,sub} (0xb10c)
Pull request description:
Since ea7b8528 (#26422), `autogen.sh` overwrites the `build-aux/config.{guess, sub}` files (installed there by `autoreconf`) with the `depends/config.{guess, sub}` files if these are newer.
The `autoreconf` tool copies them from it's `share/autoconf/build-aux/` directory. Specifically on NixOS, the `share/autoconf/build-aux/` files are located in the nix-store and are read-only. `autoreconf` preserves the read-only permissions when copying. Overwriting them with our `depends/config.{guess, sub}` files subsequently fails.
To make sure we can overwrite the files, set write permissions to the current user and group before overwriting. This fixes the problem on NixOS.
fixes#27873
ACKs for top commit:
dergoegge:
tACK fc6c17b83887ef193f2b97264b1843c94dcb915d
fanquake:
ACK fc6c17b83887ef193f2b97264b1843c94dcb915d
Tree-SHA512: e8a31f739d5b598b2fe9fe6fc3d02303c117a6adccc49b8d0fea4980027a64f915a0e1e00e4788dce6113ef1b9ec9acf9e4164486f6e4904bad405f20b6746a0
After initially being merged in #20487, it's no-longer clear that an
internal syscall sandboxing mechanism is something that Bitcoin Core
should have/maintain, especially when compared to better
maintained/supported alterantives, i.e firejail.
Note that given where it's used, the sandbox also gets dragged into the
kernel.
There is some related discussion in #24771.
This should not require any sort of deprecation, as this was only ever
an opt-in, experimental feature.
Closes#24771.
Currently InvalidateCoinsDBOnDisk is calling AbortNode without an error to the
caller if it fails. Change it to return just return util::Result, and update
the caller to handle the error itself.
This causes the secondary error to be shown below the main error instead of the
other way around.
a97c59f12d50d11d8859f4bbfb9fcf66de667ca0 test: p2p: check misbehavior for non-continuous headers messages (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for a peer sending a `headers` message where the headers don't connect to each other, which should be treated as misbehaving (not disconnecting though, as the score increase is only 20). The relevant code path is `PeerManagerImpl::ProcessHeadersMessage` -> `PeerManagerImpl::CheckHeadersPoW` -> `PeerManagerImpl::CheckHeadersAreContinuous`:
17acb2782a/src/net_processing.cpp (L2415-L2419)17acb2782a/src/net_processing.cpp (L2474-L2484)
ACKs for top commit:
sr-gi:
ACK a97c59f12d
achow101:
ACK a97c59f12d50d11d8859f4bbfb9fcf66de667ca0
instagibbs:
ACK a97c59f12d50d11d8859f4bbfb9fcf66de667ca0
Tree-SHA512: 3f8d6a2492e5c8b63c7b11be2e4ec455f83581b2c58f2d4e705baadfe8d7c6377296d6cd0eda679d291a13d8930b09443f8e3d183795df34b780c703d5d3aeb3
Make LoadChainstate return an explicit error when snapshot validation succeeds,
but there is an error trying to replace the background chainstate with the
snapshot chainstate. Previously in this case LoadChainstate would trigger a
shutdown and return INTERRUPTED, now it will return an actual error code.
There's no real change to behavior other than error message being formatted a
little differently.
Motivation for this change is to replace error handling via callbacks with
error handling via return value ahead of
https://github.com/bitcoin/bitcoin/pull/27861
fa8ef7d138913d2f10482b0f1693ad94ce497f11 refactor: Avoid copy of bilingual_str when formatting, Fix ADL violation (MarcoFalke)
Pull request description:
This refactor shouldn't change behavior, but may fix compile errors such as https://github.com/bitcoin/bitcoin/pull/27862#issuecomment-1592516184
ACKs for top commit:
achow101:
ACK fa8ef7d138913d2f10482b0f1693ad94ce497f11
ryanofsky:
Code review ACK fa8ef7d138913d2f10482b0f1693ad94ce497f11. Looks great! Thanks for updating
hebasto:
ACK fa8ef7d138913d2f10482b0f1693ad94ce497f11, I have reviewed the code and it looks OK.
Tree-SHA512: 903019962f27b5432b8e3af052b472238ef68d3ee165148c9d2232bf290309075f9f17d8d06c9b5c7fddb89c1a9c3a4c09c6310af01e8561adc0244a30db0857
When processing `CMPCTBLOCK` message, at some moments
we can need to process cmpct block txns, since all messages
are handled by ProcessMessage, we call ProcessMessage
all over again. For this reason, it creates a function called
`ProcessCompactBlockTxns` to process it.
6779e6ed7f9ad455566dca8c5aa4ef13b6715cd5 test: clean up is node stopped (dimitaracev)
Pull request description:
Fixes#27893
Use f'strings for the message when asserting `expected_ret_code` and `return_code`. Change the `expected_ret_code` from an optional to have a default value of `0`.
cc MarcoFalke
ACKs for top commit:
MarcoFalke:
lgtm ACK 6779e6ed7f9ad455566dca8c5aa4ef13b6715cd5
stickies-v:
ACK 6779e6ed7f9ad455566dca8c5aa4ef13b6715cd5
brunoerg:
ACK 6779e6ed7f9ad455566dca8c5aa4ef13b6715cd5
Tree-SHA512: af84e7ffe467ced29236dee9206687786a2efb89ab8b039c3ebfb93ea23fc273206cd51f20c9fb6bee4135770e9a649538942571d9c0be83ba9535fa8e59cb28
The return type of TranslateArg is std::string, which creates a copy.
Fix this by moving everything into a lambda that takes a reference and
returns a reference.
Also, the format function is called without specifying the namespace it
lives in. Fix this by specifying the namespace. See also:
7a59865793/doc/developer-notes.md (L117-L137).
3b2acfcfec83a4e6e50b3f21e0810274bdb05afb build: suppress external warnings by default (fanquake)
Pull request description:
I think we are at the point where it make more sense to make this the default, than not. It's already used in the CI, and I assume most building locally are also utilising it.
ACKs for top commit:
achow101:
ACK 3b2acfcfec83a4e6e50b3f21e0810274bdb05afb
hebasto:
ACK 3b2acfcfec83a4e6e50b3f21e0810274bdb05afb
stickies-v:
ACK 3b2acfcfec83a4e6e50b3f21e0810274bdb05afb
Tree-SHA512: be20203381c03dea8b5d64876c56bf8bb8defdfd6fc6d5398b71d3f28d0209c4bd1374f108df708aaa8867fda818a9bc611d1908c9fbb74f8cccdfbc5aff05af
162602b208493e7da1984044e661402c3e52932b fuzz: wallet, add target for `fees` (brunoerg)
Pull request description:
This PR adds fuzz coverage for `wallet/fees`. Some functions may use or not (non default) values from `wallet`, `CCoinControl` or `FeeCalculation`. So the logic is to make the test sometimes fill up some attributes and others no.
Obs: As soon as this PR gets some reviews, I can open the proper PR to `qa-assets` as well.
ACKs for top commit:
Xekyo:
ACK 162602b208493e7da1984044e661402c3e52932b
MarcoFalke:
lgtm ACK 162602b208493e7da1984044e661402c3e52932b
dergoegge:
Code review ACK 162602b208493e7da1984044e661402c3e52932b
Tree-SHA512: 6545802f27aafb60bf5a119af514e9425b643780dea6650bba766bb5be813f2aaddb7afc7f0efa2943ceb26f5ea08b42c95a3c0df897493c71f2d2f99e9e4236
This commit adds tests to ensure that old fee_estimates.dat files
are not read and that fee_estimates are periodically flushed to the
fee_estimates.dat file.
Additionaly it tests the -regtestonly option -acceptstalefeeestimates.
If -acceptstalefeeestimates option is passed stale fee estimates can now
be read when operating in regtest environments.
Additionally, this commit updates all declarations of the CBlockPolicyEstimator
class to include a the second constructor variable.
This is a refactor as long as no signed integer overflow appears. In
normal operation and absent bugs, signed integer overflow should never
happen in the touched code paths.
The main benefit of this refactor is to drop the file-wide ubsan
suppression unsigned-integer-overflow:txmempool.cpp.
For now, this only changes the internal private representation and the
publicly returned type remains uint64_t.
