b01f336708 util, refactor: Drop explicit conversion to fs::path (Hennadii Stepanov)
138c668e2b util, refactor: Use GetPathArg to read "-rpccookiefile" value (Hennadii Stepanov)
1276090705 util, refactor: Use GetPathArg to read "-conf" value (Hennadii Stepanov)
Pull request description:
This PR is a continuation of bitcoin/bitcoin#24265 and bitcoin/bitcoin#24306.
Now the following command-line arguments / configure options been read with the `GetPathArg` method:
- `-conf`, also `includeconf` values been normalized
- `-rpccookiefile`
ACKs for top commit:
jarolrod:
Code Review ACK b01f336708
ryanofsky:
Code review ACK b01f336708. Changes since last review: just dropping first commit (NormalizedPathFromString) as suggested
Tree-SHA512: 2d26d50b73542acdbcc63a32068977b2a49a017d31ca337471a0446f964eb0a6e3e4e3bb1ebe6771566a260f2cae3bc2ebe93b4b523183cea0d51768daab85c9
Our RBF policy is different from the rules specified in BIP125. For
example, the BIP does not mention Rule 6, and our Rule 4 uses the
(configurable) incremental relay feerate (distinct from the
minimum relay feerate). Those interested in our policy should refer to
doc/policy/mempool-replacements.md instead. These rules may also
continue to diverge with package RBF and other RBF improvements. Keep
references to the BIP125 signaling wrt sequence numbers, since that is
still correct and widely used. It is helpful to refer to this as "BIP125
signaling" since it is unambiguous and succint, especially if we have
multiple ways to signal replaceability in the future.
The rule numbers in doc/policy/mempool-replacements.md correspond
largely to those of BIP 125, so we can still refer to them like "Rule 5."
faab8dceb3 Remove unused SetTip(nullptr) code (MacroFake)
Pull request description:
Now that this path is no longer used after commit b51e60f914, we can remove it.
Future code should reset `CChain` by simply discarding it and constructing a fresh one.
ACKs for top commit:
ryanofsky:
Code review ACK faab8dceb3. Just moved an assert statement since last review
Tree-SHA512: 7dc273b11133d85d32ca2a69c0c7c07b39cdd338141ef5b51496e7de334a809864d5459eb95535497866c8b1e468aae84ed8f91b543041e6ee20130d5622874e
0f3a2532c3 validationcaches: Use size_t for sizes (Carl Dong)
41c5201a90 validationcaches: Add and use ValidationCacheSizes (Carl Dong)
82d3058539 cuckoocache: Check for uint32 overflow in setup_bytes (Carl Dong)
b370164b31 validationcaches: Abolish arbitrary limit (Carl Dong)
08dbc6ef72 cuckoocache: Return approximate memory size (Carl Dong)
0dbce4b103 tests: Reduce calls to InitS*Cache() (Carl Dong)
Pull request description:
This is part of the `libbitcoinkernel` project: #24303, https://github.com/bitcoin/bitcoin/projects/18
This PR is **_NOT_** dependent on any other PRs.
-----
a.k.a. "Stop calling `gArgs.GetIntArg("-maxsigcachesize")` from validation code"
This PR introduces the `ValidationCacheSizes` struct and its corresponding `ApplyArgsManOptions` function, removing the need to call `gArgs` from `Init{Signature,ScriptExecution}Cache()`. This serves to further decouple `ArgsManager` from `libbitcoinkernel` code.
More context can be gleaned from the commit messages.
ACKs for top commit:
glozow:
re ACK 0f3a2532c3
theStack:
Code-review ACK 0f3a2532c3
ryanofsky:
Code review ACK 0f3a2532c3. Rebase and comment tweak since last
Tree-SHA512: a492ca608466979807cac25ae3d8ef75d2f1345de52a156aa0d222c5a940f79f1b65db40090de69183cccdb12297ec060f6c64e57a26a155a94fec80e07ea0f7
Similar to 8588591965.
```bash
ERROR: test_revocation_mode_soft (tests.test_validate.ValidateTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/tmp/guix-build-python-certvalidator-0.1-1.a145bf2.drv-0/source/tests/test_validate.py", line 85, in test_revocation_mode_soft
validate_path(context, path)
File "/tmp/guix-build-python-certvalidator-0.1-1.a145bf2.drv-0/source/tests/../certvalidator/validate.py", line 50, in validate_path
return _validate_path(validation_context, path)
File "/tmp/guix-build-python-certvalidator-0.1-1.a145bf2.drv-0/source/tests/../certvalidator/validate.py", line 358, in _validate_path
raise PathValidationError(pretty_message(
certvalidator.errors.PathValidationError: The path could not be validated because the end-entity certificate expired 2022-07-27 12:00:00Z
```
8df063e537 build: Fix help string for `--enable-external-signer` configure option (Hennadii Stepanov)
Pull request description:
This PR is a follow up of bitcoin/bitcoin#24065 and fixes the help string according to the actual default value 816ca01650/configure.ac (L324-L327)
ACKs for top commit:
kristapsk:
cr utACK 8df063e537
jarolrod:
ACK 8df063e537
Tree-SHA512: ad3f457a53c9238ddd8ded9efd1224e564e6cb9da8b7ff7733a11e32a7daad5c0f6c6223509218f44944a874470cb0d2447897662eaf4e78c763b30785717c50
For some reason, the primary consumer of getWalletTxs requires the
transactions to be in hash order when it is processing them. std::map
will iterate in hash order so the transactions end up in that order when
placed into the vector. To ensure this order when mapWallet is no longer
ordered, the vector is replaced with a set which will maintain the hash
order.
In order to avoid constantly re-deriving the same keys in
DescriptorScriptPubKeyMan, cache the SigningProviders generated inside
of GetSigningProvider.
Also:
- Make DEFAULT_MAX_SIG_CACHE_SIZE into constexpr
DEFAULT_MAX_SIG_CACHE_BYTES to utilize the compile-time integer
arithmetic overflow checking available to constexpr.
- Fix comment (MiB instead of MB) for DEFAULT_MAX_SIG_CACHE_BYTES.
- Pass in max_size_bytes parameter to InitS*Cache(), modify log line to
no longer allude to maxsigcachesize being split evenly between the two
validation caches.
- Fix possible integer truncation and add a comment.
[META] I've kept the integer types as int64_t in order to not introduce
unintended behaviour changes, in the next commit we will make
them size_t.
This fixes an potential overflow which existed prior to this patchset.
If CuckooCache::cache<Element, Hash>::setup_bytes is called with a
`size_t bytes` which, when divided by sizeof(Element), does not fit into
an uint32_t, the implicit conversion to uint32_t in the call to setup
will result in an overflow.
At least on x86_64, this overflow is possible:
static_assert(std::numeric_limits<size_t>::max() / 32 <= std::numeric_limits<uint32_t>::max());
static_assert(std::numeric_limits<size_t>::max() / 4 <= std::numeric_limits<uint32_t>::max());
This commit detects such cases and signals to callers that the `size_t
bytes` input is too large.
1. -maxsigcachesize is a DEBUG_ONLY option
2. Almost 7 years has passed since its semantics change in
830e3f3d02 from "number of entries" to
"number of mebibytes"
3. A std::new_handler was added to the codebase after the original PR
which introduced this limit, which will terminate immediately instead
of causing trouble by being caught somewhere unexpected.
Returning the approximate total size eliminates the need for
InitS*Cache() to do nElems*sizeof(uint256). The cuckoocache has a better
idea of this information.
In src/test/fuzz/script_sigcache.cpp, we should really be setting up a
full working BasicTestingSetup. The initialize_ function is only run
once anyway.
In src/test/txvalidationcache_tests.cpp, the Dersig100Setup inherits
from BasicTestingSetup, which should have already set up a global script
execution cache without the need to explicitly call
InitScriptExecutionCache.
Our RBF policy is different from the rules specified in BIP125 (refer to
doc/policy/mempool-replacements.md instead), and will continue to
diverge with package RBF. Keep references to BIP125 sequence number,
since that is still useful and correct.
-BEGIN VERIFY SCRIPT-
ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); }
ren m_allow_bip125_replacement m_allow_replacement
ren allow_bip125_replacement allow_replacement
ren MAX_BIP125_REPLACEMENT_CANDIDATES MAX_REPLACEMENT_CANDIDATES
-END VERIFY SCRIPT-
Rename `BResult` class to `util::Result` and update the class interface to be
more compatible with `std::optional` and with a full-featured result class
implemented in https://github.com/bitcoin/bitcoin/pull/25665. Motivation for
this change is to update existing `BResult` usages now so they don't have to
change later when more features are added in #25665.
This change makes the following improvements originally implemented in #25665:
- More explicit API. Drops potentially misleading `BResult` constructor that
treats any bilingual string argument as an error. Adds `util::Error`
constructor so it is never ambiguous when a result is being assigned an error
or non-error value.
- Better type compatibility. Supports `util::Result<bilingual_str>` return
values to hold translated messages which are not errors.
- More standard and consistent API. `util::Result` supports most of the same
operators and methods as `std::optional`. `BResult` had a less familiar
interface with `HasRes`/`GetObj`/`ReleaseObj` methods. The Result/Res/Obj
naming was also not internally consistent.
- Better code organization. Puts `src/util/` code in the `util::` namespace so
naming reflects code organization and it is obvious where the class is coming
from. Drops "B" from name because it is undocumented what it stands for
(bilingual?)
- Has unit tests.
fad5bc432b test: Add missing static to IsStandardTx helper (MacroFake)
Pull request description:
Requested in https://github.com/bitcoin/bitcoin/pull/25648#discussion_r935985961
Also remove line break from the other two helpers.
ACKs for top commit:
glozow:
utACK fad5bc432b
aureleoules:
ACK fad5bc432b.
theStack:
ACK fad5bc432b
Tree-SHA512: 771411e1fb5939a58491ecf719e1929ab0150b0faae2078ac72bd13117f1d4dcffdeed5027bfae53e4336af25a4f1db47d564abc06a5a2c9ec006a9f67bae104
f2f6068b69 test: MiniWallet: add `send_self_transfer_chain` to create chain of txns (Andreas Kouloumos)
1d6b438ef0 test: use MiniWallet to simplify mempool_package_limits.py tests (Andreas Kouloumos)
Pull request description:
While `wallet.py` includes the MiniWallet class and some helper methods, it also includes some methods that have been moved there without having any direct relation with the MiniWallet class. Specifically `make_chain`, `create_child_with_parents` and `create_raw_chain` methods that were extracted from `rpc_packages.py` at f8253d69d6 in order to be used on both `mempool_package_limits.py` and `rpc_packages.py`.
Since that change, due to the introduction of additional methods in MiniWallet, the functionality of those methods can now be replicated with the existing MiniWallet methods and simultaneously simplify those tests by using the MiniWallet.
This PR's goals are
- to simplify the `mempool_package_limits.py` functional tests with usage of the MiniWallet.
- to make progress towards the removal of the `make_chain`, `create_child_with_parents` and `create_raw_chain` methods of `wallet.py`.
For the purpose of the aforementioned goals, a helper method `MiniWallet.send_self_transfer_chain` is introduced and method `bulk_transaction` has been integrated in `create_self_transfer*` methods using an optional `target_weight` option.
ACKs for top commit:
MarcoFalke:
ACK f2f6068b69👜
Tree-SHA512: 3ddfa0046168cbf7904ec6b1ca233b3fdd4f30db6aefae108b6d7fb69f34ef6fb2cf4fa7cef9473ce1434a0cc8149d236441a685352fef35359a2b7ba0d951eb
ddddd6913b sort after scripted-diff (MacroFake)
fac812ca83 scripted-diff: Move mempool_args to src/node (MacroFake)
66664384a6 Remove ::g_max_datacarrier_bytes global (MacroFake)
fad0b4fab8 Pass datacarrier setting into IsStandard (MacroFake)
fa2a6b8516 Combine datacarrier globals into one (MacroFake)
fa477d32ee Remove ::GetVirtualTransactionSize() alias (MacroFake)
fa2f6c1a61 Remove ::fIsBareMultisigStd global (MacroFake)
fadc14e4f5 Remove ::dustRelayFee (MacroFake)
fa8a7f01fe Remove ::IsStandardTx(tx, reason) alias (MacroFake)
fa7a9114e5 test: Remove unused cs_main (MacroFake)
fa9cba7afb Remove ::incrementalRelayFee and ::minRelayTxFee globals (MacroFake)
fa148602e6 Remove ::fRequireStandard global (MacroFake)
fa468bdfb6 Return optional error from ApplyArgsManOptions (MacroFake)
Pull request description:
This change is good because:
* It moves module-specific init-logic out of the bloated init.cpp
* It removes a global from validation.cpp and places it into the data structure that needs it (mempool)
ACKs for top commit:
glozow:
re ACK ddddd69
ryanofsky:
Code review ACK ddddd6913b
ariard:
Light Code Review ACK ddddd69
Tree-SHA512: 9de2ce601cfcaa4dfd7d1c92270568895ce8702ccdffb59829fbe9618eab0fd88d738afef33ed66988c66861115e0340e881056bfb71e2aed4af2440bd37eb1e
9e04cfaa76 test: add coverage for wallet inconsistent state during sync (furszy)
77de5c693f wallet: guard and alert about a wallet invalid state during chain sync (furszy)
Pull request description:
Follow-up work to my comment in #25239.
Guarding and alerting the user about a wallet invalid state during chain synchronization.
#### Explanation
if the `AddToWallet` tx write fails, the method returns a wtx `nullptr` without removing the recently added transaction from the wallet's map.
Which makes that `AddToWalletIfInvolvingMe` return false (even when the tx is on the wallet's map already), --> which makes `SyncTransaction` skip the `MarkInputsDirty` call --> which leads to a wallet invalid state where the inputs of this new transaction are not marked dirty, while the transaction that spends them still exist on the in-memory wallet tx map.
Plus, as we only store the arriving transaction inside `AddToWalletIfInvolvingMe` when we synchronize/scan block/s from the chain and nowhere else, it makes sense to treat the transaction db write error as a runtime error to notify the user about the problem. Otherwise, the user will lose all the not stored transactions after a wallet shutdown (without be able to recover them automatically on the next startup because the chain sync would be above the block where the txs arrived).
Note:
On purpose, the first commit adds test coverage for it. Showing how the wallet can end up in an invalid state. The second commit corrects it with the proposed solution.
ACKs for top commit:
achow101:
re-ACK 9e04cfaa76
jonatack:
ACK 9e04cfaa76
Tree-SHA512: 81f765eca40547d7764833d8ccfae686b67c7728c84271bc00dc51272de643dafc270014079dcc9727b47577ba67b340aeb5f981588b54e69a06abea6958aa96
It is part of the node library. Also, it won't be moved to the kernel
lib, as it will be pruned of ArgsManager.
-BEGIN VERIFY SCRIPT-
# Move module
git mv src/mempool_args.cpp src/node/
git mv src/mempool_args.h src/node/
# Replacements
sed -i 's:mempool_args\.h:node/mempool_args.h:g' $(git grep -l mempool_args)
sed -i 's:mempool_args\.cpp:node/mempool_args.cpp:g' $(git grep -l mempool_args)
sed -i 's:MEMPOOL_ARGS_H:NODE_MEMPOOL_ARGS_H:g' $(git grep -l MEMPOOL_ARGS_H)
-END VERIFY SCRIPT-
fa7bef2e80 univalue: Remove unused and confusing set*() return value (MacroFake)
Pull request description:
The value is:
* currently unused, and useless without `[[nodiscard]]`
* confusing, because it is always `true`, unless a num-string is set
Instead of adding `[[nodiscard]]`, throw when setting is not possible.
ACKs for top commit:
shaavan:
ACK fa7bef2e80
aureleoules:
ACK fa7bef2e80.
Tree-SHA512: 0d74f96f34cb93b66019ab75e12334c964630cc83434f22e58cc7a4fff2ee96a5767e42ab37f08acb67aeacba6811b09c75f1edc68d5e903ccfc59b1c82de891
