fa63f16018 test: Add uint256 string parse tests (MarcoFalke)
facf629ce8 refactor: Remove unused and fragile string interface from arith_uint256 (MarcoFalke)
Pull request description:
The string interface (`base_uint(const std::string&)`, as well as `base_uint::SetHex`) is problematic for many reasons:
* It is unused (except in test-only code).
* It is redundant with the `uint256` string interface: `std::string -> uint256 -> UintToArith256`.
* It is brittle, because it inherits the brittle `uint256` string interface, which is brittle due to the use of `c_str()` (embedded null will be treated as end-of string), etc ...
Instead of fixing the interface, remove it since it is unused and redundant with `UintToArith256`.
ACKs for top commit:
ajtowns:
ACK fa63f16018
TheCharlatan:
ACK fa63f16018
Tree-SHA512: a95d5b938ffd0473361336bbf6be093d01265a626c50be1345ce2c5e582c0f3f73eb11af5fd1884019f59d7ba27e670ecffdb41d2c624ffb9aa63bd52b780e62
All functions assume that the pointer is never null, so pass by
reference, to avoid accidental segfaults at runtime, or at least make
them more obvious.
Also, remove unused c-style casts in touched lines.
Also, add CHECK_NONFATAL checks, to turn segfault crashes into an
recoverable runtime error with debug information.
fad1903b8a fuzz: Avoid timeout in bitdeque (MarcoFalke)
Pull request description:
Avoid timeouts such as https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1842914664
This is done by:
* Limiting the maximum number of iterations if the maximum size of the container is "large" (see the magic numbers in the code).
* Check the equality only once. This should be fine, because if a crash were to happen in the equality check, but the crash doesn't happen if further iterations were run, the fuzz engine should eventually find the crash by truncating the fuzz input.
ACKs for top commit:
sipa:
utACK fad1903b8a
dergoegge:
utACK fad1903b8a
brunoerg:
crACK fad1903b8a
Tree-SHA512: d3d83acb3e736b8fcaf5d17ce225ac82a9f9a2efea048512d2fed594ba6c76c25bae72eb0fab3276d4db37baec0752e5367cecfb18161301b921fed09693045e
3ea54e5db7 net: Add continuous ASMap health check logging (Fabian Jahr)
28d7e55dff test: Add tests for unfiltered GetAddr usage (Fabian Jahr)
b8843d37ae fuzz: Let fuzzers use filter options in GetAddr/GetAddresses (Fabian Jahr)
e16f420547 net: Optionally include terrible addresses in GetAddr results (Fabian Jahr)
Pull request description:
There are certain statistics we can collect by running all our known clearnet addresses against the ASMap file. This could show issues with a maliciously manipulated file or with an old file that has decayed with time.
This is just a proof of concept for now. My idea currently is to run the analysis once per day and print the results to logs if an ASMap file is used.
ACKs for top commit:
achow101:
ACK 3ea54e5db7
mzumsande:
ACK 3ea54e5db7
brunoerg:
crACK 3ea54e5db7
Tree-SHA512: 777acbfac43cc43ce4a0a3612434e4ddbc65f59ae8ffc9e24f21de09011bccb297f0599cbaa82bcf40ef68e5af582c4e98556379db7ceff7d9f97574a1cf8e09
ca09415e63 rpc, doc: encryptwallet, mention HD seed rotation and new backup (furszy)
Pull request description:
Small and simple PR, updating the `encryptwallet` help message.
Better to notify users about the HD seed rotation and the new
backup requirement before executing the encryption process.
Ensuring they are prepared to update previous backups and
securely safeguard the updated wallet file.
ACKs for top commit:
S3RK:
ACK ca09415e63
achow101:
ACK ca09415e63
Tree-SHA512: f0ee65f5cea66450566e3a85e066d4c06b3293dd0e0b2ed5fafdb7fb11da0a2cd94407299a3c57a0706c2ed782f8eabb73443e85d8099a62a3fb10a02636ab46
55e3dc3e03 test: Fix test by checking the actual exception instance (Hennadii Stepanov)
Pull request description:
The `system_tests/run_command` test is broken because it passes even with the diff as follows:
```diff
--- a/src/test/system_tests.cpp
+++ b/src/test/system_tests.cpp
@@ -90,7 +90,7 @@ BOOST_AUTO_TEST_CASE(run_command)
});
}
{
- BOOST_REQUIRE_THROW(RunCommandParseJSON("echo \"{\""), std::runtime_error); // Unable to parse JSON
+ BOOST_REQUIRE_THROW(RunCommandParseJSON("invalid_command \"{\""), std::runtime_error); // Unable to parse JSON
}
// Test std::in, except for Windows
#ifndef WIN32
```
The reason of such fragility is that the [`BOOST_REQUIRE_THROW`](https://www.boost.org/doc/libs/1_83_0/libs/test/doc/html/boost_test/utf_reference/testing_tool_ref/assertion_boost_level_throw.html) macro passes even if the command raises an exception in the underlying subprocess implementation, which might have a type derived from `std::runtime_error`.
ACKs for top commit:
maflcko:
lgtm ACK 55e3dc3e03
achow101:
ACK 55e3dc3e03
furszy:
Non-Windows code ACK 55e3dc3e
pablomartin4btc:
ACK 55e3dc3e03
Tree-SHA512: 32f49421bdcc94744c81e82dc10cfa02e3f8ed111974edf1c2a47bdaeb56d7baec1bede67301cc89464fba613029ecb131dedc6bc5948777ab52f0f12df8bfe9
To avoid scanning blocks, as assumed by a wallet with no
generated keys or imported scripts, the default value for
the birth time needs to be set to the maximum int64_t value.
Once the first key is generated or the first script is imported,
the legacy SPKM will update the birth time automatically.
Better to notify users about the HD seed rotation and the new
backup requirement before executing the encryption process.
Ensuring they are prepared to update previous backups and
securely safeguard the updated wallet file.
Co-authored-by: jonatack <jon@atack.com>
38816ff64e fuzz: txorphan check wtxids using GenTxid::Wtxid not GenTxid::Txid (Greg Sanders)
Pull request description:
Fixes the bugs in the fuzz test with no more changes as an alternative to https://github.com/bitcoin/bitcoin/pull/28658
ACKs for top commit:
naumenkogs:
ACK 38816ff64e
dergoegge:
ACK 38816ff64e
Tree-SHA512: 5e46a83f2b2a2ac0672a63eb6200b019e01089ab1aa80c4ab869b6fcf27ccf2e84a064e96397f1a1869ccfa43b0c9638cbae681a27c4ca3c96ac71f41262601e
This change is mostly a refectoring that removes some code and gets rid of an
unnecessary layer of indirection after #27861
But it is not a pure refactoring since StartShutdown, AbortShutdown, and
WaitForShutdown functions used to abort on failure, and the replacement code
logs or returns errors instead.
Having InitContext() avoids the need to add duplicate code to src/init/*.cpp
files in the next commit. It also lets these files avoid referencing global
variables like gArgs.
There is no change in behavior in this commit.
Use SignalInterrupt object instead. There is a slight change in behavior here
because the previous StartShutdown code used to abort on failure and the
new code logs errors instead.
Use SignalInterrupt object instead. There is a slight change in behavior here
because the previous StartShutdown code used to abort on failure and the
new code returns an RPC error instead.
Replace exceptions thrown by signal and wait methods with [[nodiscard]] return
values.
This is mostly a refactoring, but there is a slight change of behavior if
AbortShutdown function fails. The original behavior which was unintentionally
changed in #27861 is restored, so it now triggers an assert failure again
instead of throwing an exception. (The AbortShutdown function is only ever
called in the the GUI version of Bitcoin Core when corruption is detected on
loading and the user tries to reindex.)
Problems with using exceptions were pointed out by MarcoFalke in
https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1255496707.
Pass HTTP server an interrupt object instead of having it depend on shutdown.h
and global shutdown state.
There is no change in behavior in this commit.
Add NodeContext::shutdown variable and start using it to replace the
kernel::Context::interrupt variable. The latter can't easily be removed right
away but will be removed later in this PR.
Moving the interrupt object from the kernel context to the node context
increases flexibility of the kernel API so it is possible to use multiple
interrupt objects, or avoid creating one if one is not needed. It will also
allow getting rid of the kernel::g_context global later in this PR, replacing
it with a private SignalInterrupt instance in init.cpp
There is no change in behavior in this commit outside of unit tests. In unit
tests there should be no visible change either, but internally now each test
has its own interrupt variable so the variable will be automatically reset
between tests.
Use interfaces::Node object instead.
There is a minor change in behavior in this commit, because the new code calls
InterruptRPC() and StopRPC() when previous code did not do this. But this
should be a good thing since it makes sense to interrupt RPC when the system is
shutting down, and it is better for the GUI shut down in a consistent way
regardless of how the shutdown is triggered.
Previously, starting a second bitcoind using the same datadir would
correctly fail to init and shutdown. However during shutdown the PID
file belonging to the first instance would be erroneously removed by
the second process shutting down.
Fix this to only delete the PID file if we created it.
The BOOST_REQUIRE_THROW passes even if the command raises an exception
in the underlying subprocess implementation, which might have a type
derived from std::runtime_error.
91504cbe0d rpc: `SyncWithValidationInterfaceQueue` on fee estimation RPC's (ismaelsadeeq)
714523918b tx fees, policy: CBlockPolicyEstimator update from `CValidationInterface` notifications (ismaelsadeeq)
dff5ad3b99 CValidationInterface: modify the parameter of `TransactionAddedToMempool` (ismaelsadeeq)
91532bd382 tx fees, policy: update `CBlockPolicyEstimator::processBlock` parameter (ismaelsadeeq)
bfcd401368 CValidationInterface, mempool: add new callback to `CValidationInterface` (ismaelsadeeq)
0889e07987 tx fees, policy: cast with static_cast instead of C-Style cast (ismaelsadeeq)
a0e3eb7549 tx fees, policy: bugfix: move `removeTx` into reason != `BLOCK` condition (ismaelsadeeq)
Pull request description:
This is an attempt to #11775
This Pr will enable fee estimator to listen to ValidationInterface notifications to process new transactions added and removed from the mempool.
This PR includes the following changes:
- Added a new callback to the Validation Interface `MempoolTransactionsRemovedForConnectedBlock`, which notifies listeners about the transactions that have been removed due to a new block being connected, along with the height at which the transactions were removed.
- Modified the `TransactionAddedToMempool` callback parameter to include additional information about the transaction needed for fee estimation.
- Updated `CBlockPolicyEstimator` to process transactions using` CTransactionRef` instead of `CTxMempoolEntry.`
- Implemented the `CValidationInterface` interface in `CBlockPolicyEstimater` and overridden the `TransactionAddedToMempool`, `TransactionRemovedFromMempool`, and `MempoolTransactionsRemovedForConnectedBlock` methods to receive updates from their notifications.
Prior to this PR, the fee estimator updates from the mempool, i.e whenever a new block is connected all transactions in the block that are in our mempool are going to be removed using the `removeForBlock` function in `txmempool.cpp`.
This removal triggered updates to the fee estimator. As a result, the fee estimator would block mempool's `cs` until it finished updating every time a new block was connected.
Instead of being blocked only on mempool tx removal, we were blocking on both tx removal and fee estimator updating.
If we want to further improve fee estimation, or add heavy-calulation steps to it, it is currently not viable as we would be slowing down block relay in the process
This PR is smaller in terms of the changes made compared to #11775, as it focuses solely on enabling fee estimator updates from the validationInterface/cscheduler thread notifications.
I have not split the validation interface because, as I understand it, the rationale behind the split in #11775 was to have `MempoolInterface` signals come from the mempool and `CValidationInterface` events come from validation. I believe this separation can be achieved in a separate refactoring PR when the need arises.
Also left out some commits from #11775
- Some refactoring which are no longer needed.
- Handle reorgs much better in fee estimator.
- Track witness hash malleation in fee estimator
I believe they are a separate change that can come in a follow-up after this.
ACKs for top commit:
achow101:
ACK 91504cbe0d
TheCharlatan:
Re-ACK 91504cbe0d
willcl-ark:
ACK 91504cbe0d
Tree-SHA512: 846dfb9da57a8a42458827b8975722d153907fe6302ad65748d74f311e1925557ad951c3d95fe71fb90ddcc8a3710c45abb343ab86b88780871cb9c38c72c7b1
7cb9367157 rpc: keep .cookie if it was not generated (Roman Zeyde)
Pull request description:
Otherwise, starting bitcoind twice may cause the `.cookie` file generated by the first instance to be deleted by the second instance shutdown (after failing to obtain a lock).
ACKs for top commit:
willcl-ark:
re-ACK 7cb9367157
achow101:
ACK 7cb9367157
kristapsk:
re-ACK 7cb9367157
stickies-v:
ACK 7cb9367157
Tree-SHA512: 0960dbc457975b0e0535f3d814824a879d7f85c9f1191537415b3fc253429a316a8e4badde56c8bc139778f132392983cec5fbe03891fb15ff61d3bc3f6e681b
f23ba24aa0 test_submitpackage: only make a chain of 3 txns (Greg Sanders)
e67a345162 doc: submitpackage vsize results are sigops-adjusted (Greg Sanders)
b67db52c39 RPC submitpackage: change return format to allow partial errors (Greg Sanders)
Pull request description:
This was prompted by errors being returned that didn't "make any sense" to me, because it would for example return a "fee too low" error, when the "real" error was the child had something invalid, which disallowed CPFP evaluation. Rather than make judgment calls on what error is important(which is currently just return the "first"!), we simply return all errors and let the callers determine what's best.
Added a top level `package_msg` for quick eye-balling of general success of the package.
This PR also fixes a couple bugs:
1) Currently we don't actually broadcast a transaction, even if it was entered into our mempool, if a subsequent transaction causes `PKG_TX` failure.
2) "other-wtxid" is uncovered by tests, but IIUC was previously required to return "fees" and "vsize" results, but did not. I just make those results optional.
ACKs for top commit:
Sjors:
Light re-utACK f23ba24aa0
achow101:
ACK f23ba24aa0
glozow:
utACK f23ba24aa0, thanks for taking the suggestions
Tree-SHA512: ebfd716a4fed9e8c2dea3d2181ba6a6171b06718d29ac2324c67b7a30b374d199f7e1739f91ab5d036be172d0479de9bc89c32263ee62143c0338b9b622d0cca
fa98a097a3 Rename version.h to node/protocol_version.h (MarcoFalke)
fa4fbd5816 Remove unused version.h include (MarcoFalke)
fa0ae22ff2 Remove unused SER_NETWORK, SER_DISK (MarcoFalke)
fae00fe9c2 Remove unused CDataStream (MarcoFalke)
fa7eb4f5c3 fuzz: Drop unused version from fuzz input format (MarcoFalke)
Pull request description:
Seems odd to have code that is completely dead.
Fix this by removing all of it.
ACKs for top commit:
sipa:
utACK fa98a097a3
ajtowns:
ACK fa98a097a3
ryanofsky:
Seems odd to not code review ACK fa98a097a3 (looks good)
Tree-SHA512: 9f1b9d9f92bda0512610bda6653e892756f637860362a9abfa439faab62de233cbad94b7df78ebacc160d9667aadfed4d9df08c0edefa618c040a049050fb913
e67634ef19 fuzz: BIP324: damage ciphertext/aad in full byte range (Sebastian Falbesoner)
Pull request description:
This PR is a tiny improvement for the `bip324_cipher_roundtrip` fuzz target: currently the damaging of input data for decryption (either ciphertext or aad) only ever happens in the lower nibble within the byte at the damage position, as the bit position for the `damage_val` byte was calculated with `damage_bit & 3` (corresponding to `% 4`) rather than `damage_bit & 7` (corresponding to the expected `% 8`).
Noticed while reviewing #28263 which uses similar constructs.
ACKs for top commit:
stratospher:
ACK e67634ef.
dergoegge:
utACK e67634ef19
Tree-SHA512: 1bab4df28708e079874feee939beef45eff235215375c339decc696f4c9aef04e4b417322b045491c8aec6e88ec8ec2db564e27ef1b0be352b6ff4ed38bad49a
