| Age | Commit message (Collapse) | Author |
|---|
|
|
|
The following were unused from the node namespace:
- BLOCKFILE_CHUNK_SIZE
- nPruneTarget
- OpenBlockFile
- UNDOFILE_CHUNK_SIZE
|
|
from `ArgsManager`
cb3e9a1e3f8d72daaa361fc45dd853775e754b9d Move {Load,Dump}Mempool to kernel namespace (Carl Dong)
aa306765419f7dbea12b12e15553039835ba0e4d Move DEFAULT_PERSIST_MEMPOOL out of libbitcoinkernel (Carl Dong)
06b88ffb8ae7f2b2a93a32908cd80e77fafd270c LoadMempool: Pass in load_path, stop using gArgs (Carl Dong)
b857ac60d9a0433036519c26675378bbf56a1de1 test/fuzz: Invoke LoadMempool via CChainState (Carl Dong)
b3267258b052557fc136b9a4dcb754afb9219470 Move FopenFn to fsbridge namespace (Carl Dong)
ae1e8e37567fa603a5977d7d05105c682dd3f7db mempool: Use NodeClock+friends for LoadMempool (Carl Dong)
f9e8e5719f28d84f68f7d75e26c8e7fccac8e7d3 mempool: Improve comments for [GS]etLoadTried (Carl Dong)
813962da0b17b918941c6849996845e35d84a451 scripted-diff: Rename m_is_loaded -> m_load_tried (Carl Dong)
413f4bb52b72e082ad8716664ede48352b8e7e5a DumpMempool: Pass in dump_path, stop using gArgs (Carl Dong)
bd4407817e523e3c5b347bc6be25ed007cb27034 DumpMempool: Use std::chrono instead of weird int64_t arthmetics (Carl Dong)
c84390b741ab7b61c9f702d8b447c8cadc1257c8 test/mempool_persist: Test manual savemempool when -persistmempool=0 (Carl Dong)
Pull request description:
This is part of the `libbitcoinkernel` project: #24303, https://github.com/bitcoin/bitcoin/projects/18
-----
This PR moves `{Dump,Load}Mempool` into its own `kernel/mempool_persist` module and introduces `ArgsManager` `node::` helpers in `node/mempool_persist_args`to remove the scattered calls to `GetBoolArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)`.
More context can be gleaned from the commit messages.
-----
One thing I was reflecting on as I wrote this was that in the long run, I think we should probably invert the validation <-> mempool relationship. Instead of mempool not depending on validation, it might make more sense to have validation not depend on mempool. Not super urgent since `libbitcoinkernel` will include both validation and mempool, but perhaps something for the future.
ACKs for top commit:
glozow:
re ACK cb3e9a1e3f8d72daaa361fc45dd853775e754b9d via `git range-diff 7ae032e...cb3e9a1`
MarcoFalke:
ACK cb3e9a1e3f 🔒
ryanofsky:
Code review ACK cb3e9a1e3f8d72daaa361fc45dd853775e754b9d
Tree-SHA512: 979d7237c3abb5a1dd9b5ad3dbf3b954f906a6d8320ed7b923557f41a4472deccae3e8a6bca0018c8e7a3c4a93afecc502acd1e26756f2054f157f1c0edd939d
|
|
When a transaction arrives, the wallet mark its inputs (prev-txs) as dirty.
Clearing the wallet transaction cache, triggering a balance recalculation.
If this does not happen due a db write error during `AddToWallet`, the wallet
will be in an invalid state: The transaction that spends certain wallet UTXO will
exist inside the in-memory wallet tx map, having the credit/debit calculated,
while its inputs will still have the old cached data (like if them were never
spent).
|
|
-Context:
If `AddToWallet` db write fails, the method returns a wtx nullptr without
removing the recently added transaction from the wallet's map.
-Problem:
When a db write error occurs, `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 arriving transaction inside `AddToWalletIfInvolvingMe`
when we synchronize/scan blocks from the chain and nowhere else, it makes sense
to treat the tx 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).
|
|
2315830491b2cfa6b6e3e277700238e5ac92a8e0 fuzz: Fix assert bug in txorphan target (chinggg)
Pull request description:
Fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=48914.
It is possible to construct big tx that got rejected in `AddTx`, so we cannot assume tx will be added successfully. We can only guarantee tx will not be added if orphanage already has it.
ACKs for top commit:
MarcoFalke:
lgtm ACK 2315830491b2cfa6b6e3e277700238e5ac92a8e0
Tree-SHA512: e173bc1a932639746de1192ed238e2e2318899f55371febb598facd0e811d8c54997f074f5e761757e1ffd3ae76d8edf9d673f020b2d97d5762ac656f632be81
|
|
cached debit/credit amount
757216e31cac7dcd45e11b2a2c6148420b3b99da wallet: don't iter twice when getting the cached debit/credit amount (Antoine Poinsot)
Pull request description:
A small optimization i stumbled upon while looking at something else. Figured it could be worth a PR.
Instead of calling GetCachableAmount twice, which will result in
iterating through all the transaction txins/txouts and calling
GetDebit/GetCredit (which lock cs_wallet), just merge the filters and do
it once.
ACKs for top commit:
achow101:
ACK 757216e31cac7dcd45e11b2a2c6148420b3b99da
aureleoules:
ACK 757216e31cac7dcd45e11b2a2c6148420b3b99da.
Tree-SHA512: 0dbbdd24231380196e929dce572752e6be1d69457252a7215e279e71d6199483b516f64019ae999a91dbce7fdd86f8bf0336b6e151cca93cbcf51bc854e838a2
|
|
|
|
Can be reviewed with the git options
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
|
|
|
|
|
|
Having references to downstream no-longer make sense now that we've
unsubtree'd.
|
|
743a84a5f6f660e113574de349553144e0b490ff fix gettxout help text (Marnix)
Pull request description:
replaces #25578
Add help text to asm & hex (like everywhere else).
I've also changed two `RPCResult::Type::STR` to `RPCResult::Type::STR_HEX`
Top commit has no ACKs.
Tree-SHA512: 4109d6abddf71b24899f3252545248bb0c7cc366eb994d30927eb300d0b939a14b8140bac4a4c2bd45098a406666dbe1feb10da8dec923777bb8ed26784dfd54
|
|
|
|
2c3ee4c347838ecadb17a011932dffc077e46630 gui: Load Base64 PSBT string from file (Andrew Chow)
Pull request description:
Some .psbt files may have the PSBT as a base64 string instead of in binary. We should be able to load those files.
ACKs for top commit:
jarolrod:
tACK 2c3ee4c347838ecadb17a011932dffc077e46630
shaavan:
ACK 2c3ee4c347838ecadb17a011932dffc077e46630
Tree-SHA512: 352b0611693c8989ea7d1b8d494ea58c69dc15cf81b8d62271541832e74b0a0399cb6ed4e686ab7c741cb4e5374527e054a9ecfe7355bc6f77d8fdd13569ab76
|
|
Also:
1. Add the newly introduced kernel/mempool_persist.cpp to IWYU CI script
2. Add chrono mapping for iwyu
|
|
It is no longer used by anything inside libbitcoinkernel, move it to
node/mempool_persist_args.h where it belongs.
|
|
Also:
1. Have CChainState::LoadMempool and ::ThreadImport take in paths and
pass it through untouched to LoadMempool.
2. Make LoadMempool exit early if the load_path is empty.
3. Adjust the call to ::ThreadImport in ::AppInitMain to correctly pass
in an empty path if mempool persistence is disabled.
|
|
Not only does this increase coverage, it is also more correct in that
when ::LoadMempool is called with a mempool and chainstate, it calls
AcceptToMemoryPool with just the chainstate.
AcceptToMemoryPool will then act on the chainstate's mempool via
CChainState::GetMempool, which may be different from the mempool
originally passed to ::LoadMempool. (In this fuzz test's case, it
definitely is different)
Also, move DummyChainstate to its own file since it's now used by the
validation_load_mempool fuzz test to replace CChainState's m_mempool.
|
|
[META] In a future commit in this patchset, it will be used by more than
just validation, and it needs to align with fopen anyway.
|
|
|
|
Watchonly wallets do not have any private keys to encrypt. It does not
make sense to encrypt such wallets, so disable the option to encrypt
them.
This avoids an assertion that can be hit when encrypting watchonly descriptor
wallets.
|
|
|
|
Also change the param name for SetLoadTried to load_tried.
|
|
m_is_loaded/IsLoaded() doesn't actually indicate whether or not the
mempool was successfully, loaded, but rather if a load has been
attempted and did not result in a catastrophic ShutdownRequested.
-BEGIN VERIFY SCRIPT-
find_regex="\bm_is_loaded\b" \
&& git grep -l -E "$find_regex" \
| xargs sed -i -E "s@$find_regex@m_load_tried@g"
find_regex="\bIsLoaded\b" \
&& git grep -l -E "$find_regex" \
| xargs sed -i -E "s@$find_regex@GetLoadTried@g"
find_regex="\bSetIsLoaded\b" \
&& git grep -l -E "$find_regex" \
| xargs sed -i -E "s@$find_regex@SetLoadTried@g"
-END VERIFY SCRIPT-
|
|
pushes over silent ignore
fa277cd55dd105018e7d1220b4c3d96779e6b0f4 univalue: Throw exception on invalid pushes over silent ignore (MacroFake)
ccccc17b91698aa09ac85f7efea298f3938594ad refactor: Default options in walletcreatefundedpsbt to VOBJ instead of VNULL (MacroFake)
Pull request description:
The return value of the `push*` helpers is never used, but important to determine if the operation was successful. One way to fix this would be to add the "nodiscard" attribute. However, this would make the code (and this diff) overly verbose for no reason.
So fix it by removing the never used return value. Also, fail verbosely in case of a programming mistake.
ACKs for top commit:
furszy:
code ACK fa277cd5
Tree-SHA512: ef212a5bf5ae6bbad20acc4dafa3715521e81544185988d1eab724f440e4864a27e686aff51d5bc51b3017892c2eb8e577bcb8f37e8ddbaa0d8833bb622f2f9c
|
|
Also introduce node::{ShouldPersistMempool,MempoolPath} helper functions
in node/mempool_persist_args.{h,cpp} which are used by non-kernel
DumpMempool callers to determine whether or not to automatically dump
the mempool and where to dump it to.
|
|
This makes it so that DumpMempool doesn't depend on MICRO anymore
|
|
We currently expose a method to get the signing providers, which allows
to infer a descriptor from the scriptPubKey. But in order to identify
"on" what descriptor a coin was received, we need access to the
descriptors that were imported to the wallet.
|
|
ffc79b8e492c6dd1352e528fd82e45d8d25eaa04 qa: functional test Miniscript watchonly support (Antoine Poinsot)
bfb036756ad6e187fd6d3abfefe5804bb54a5c71 Miniscript support in output descriptors (Antoine Poinsot)
4a082887bee76a96deada5dbd7f991c23b301c54 qa: better error reporting on descriptor parsing error (Antoine Poinsot)
d25d58bf5f301d3bb8683bd67c8847a4957d8e97 miniscript: add a helper to find the first insane sub with no child (Antoine Poinsot)
c38c7c5817b7e73cf0f788855c4aba59c287b0ad miniscript: don't check for top level validity at parsing time (Antoine Poinsot)
Pull request description:
This adds Miniscript support for Output Descriptors without any signing logic (yet). See the OP of #24147 for a description of Miniscript and a rationale of having it in Bitcoin Core.
On its own, this PR adds "watchonly" support for Miniscript descriptors in the descriptor wallet. A follow-up adds signing support.
A minified corpus of Miniscript Descriptors for the `descriptor_parse` fuzz target is available at https://github.com/bitcoin-core/qa-assets/pull/92.
The Miniscript descriptors used in the unit tests here and in #24149 were cross-tested against the Rust implementation at https://github.com/rust-bitcoin/rust-miniscript.
This PR contains code and insights from Pieter Wuille.
ACKs for top commit:
Sjors:
re-utACK ffc79b8e492c6dd1352e528fd82e45d8d25eaa04
achow101:
ACK ffc79b8e492c6dd1352e528fd82e45d8d25eaa04
w0xlt:
reACK https://github.com/bitcoin/bitcoin/pull/24148/commits/ffc79b8e492c6dd1352e528fd82e45d8d25eaa04
Tree-SHA512: 02d919d38bb626d3c557eca3680ce71117739fa161b7a92cfdb6c9c432ed88870b1ed127ba24248574c40c7428217d7e9bdd986fd8cd7c51fae8c776e1271fb9
|
|
|
|
|
|
|
|
Use Peer::m_their_services instead
|
|
|
|
fClient is replaced by CanServeBlocks(), and m_limited_node is replaced
by IsLimitedPeer().
|
|
Prior to this commit, the peer was connected, and then the services and
connectivity fields in the CNode object were manually set. Instead, send
p2p `version` and `verack` messages, and have net_processing's internal
logic set the state of the node.
This ensures that the node's internal state is consistent with how it
would be set in the live code.
Prior to this commit, `dummyNode1.nServices` was set to `NODE_NONE`
which was not a problem since `CNode::fClient` and
`CNode::m_limited_node` are default initialised to false. Now that we
are doing the actual version handshake, the values of `fClient` and
`m_limited_node` are set during the handshake and cause the test to fail
if we do not set `dummyNode1.nServices` to a reasonable value
(NODE_NETWORK | NODE_WITNESS).
|
|
Track services offered by us and the peer in the Peer object.
|
|
This wasn't available at the time when ChainstateManager::Options was
introduced but is helpful to be explicit and ensure correctness.
|
|
It should have been there in the first place.
|
|
As UniValue provides several constructors for integral types, the
compiler is unable to select one if the passed type does not exactly
match. This is unintuitive for developers and forces them to write
verbose and brittle code.
For example, there are many places where an unsigned int is cast to a
signed int. While the cast is safe in practice, it is still needlessly
verbose and confusing as the value can never be negative. In fact it
might even be unsafe if the unsigned value is large enough to map to a
negative signed one.
|
|
Miniscript descriptors are defined under P2WSH context (either `wsh()`
or `sh(wsh())`).
Only sane Miniscripts are accepted, as insane ones (although valid by
type) can have surprising behaviour with regard to malleability
guarantees and resources limitations.
As Miniscript descriptors are longer and more complex than "legacy"
descriptors, care was taken in error reporting to help a user determine
for what reason a provided Miniscript is insane.
Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
|
|
A nit, but was helpful when writing unit tests for Miniscript parsing
|
|
This is helpful for finer grained descriptor parsing error: when there
are multiple errors to report in a Miniscript descriptor start with the
"smallest" fragments: the ones closer to be a leaf.
Co-Authored-By: Pieter Wuille <pieter@wuille.net>
|
|
Letting the caller perform the checks allows for finer-grained error
reporting.
|
|
UniValue does not have a constructor for enum values, however the
compiler will decay the enum into an int and select that constructor.
Avoid this compiler magic and clarify the code by explicitly selecting
the int-constructor.
This is needed for the next commit.
|
|
fa475e9c7977a952617738f2ee8cf600c07d4df8 refactor: Return BResult from restoreWallet (MacroFake)
fa8de09edc9ec4e6d171df80f746174a0ec58afb Prepare BResult for non-copyable types (MacroFake)
Pull request description:
This avoids the `error` in-out param (and if `warnings` is added to `BResult`, it will avoid passing that in-out param as well).
Also, as it is needed for this change, prepare `BResult` for non-copyable types.
ACKs for top commit:
w0xlt:
reACK https://github.com/bitcoin/bitcoin/pull/25594/commits/fa475e9c7977a952617738f2ee8cf600c07d4df8
ryanofsky:
Code review ACK fa475e9c7977a952617738f2ee8cf600c07d4df8. Changes since last review were replacing auto with explicit type and splitting commits
Tree-SHA512: 46350883572f13721ddd198f5dfb88d2fa58ebcbda416f74da3563ea15c920fb1e6ff30558526a4ac91c36c21e6afe27751a4e51b7b8bcbcbe805209f4e9014b
|
|
m_last_getheaders_timestamp
613e2211493ae2c78b71d1f4ea62661438d2cb73 test: remove unnecessary parens (Suhas Daftuar)
e939cf2b7645c2b20a68cb6129f3aebfdf287d61 Remove atomic for m_last_getheaders_timestamp (Suhas Daftuar)
Pull request description:
Eliminate the unnecessary atomic guarding `m_last_getheaders_timestamp`, which is only accessed in a single thread (thanks to MarcoFalke for pointing this out).
Also address a nit that came up in #25454.
ACKs for top commit:
MarcoFalke:
review ACK 613e2211493ae2c78b71d1f4ea62661438d2cb73
vasild:
ACK 613e2211493ae2c78b71d1f4ea62661438d2cb73
Tree-SHA512: 6d6c473735b450b8ad43aae5cf16ed419154d72f4a05c0a6ce6f26caecab9db2361041398b70bf9395611c107d50897f501fa5fdbabb2891144bbc2b479dfdad
|
|
|
|
|
