facc2fa7b8 Use AutoFile where possible (MacroFake)
6666803c89 streams: Add AutoFile without ser-type and ser-version (MacroFake)
Pull request description:
This was done in the context of https://github.com/bitcoin/bitcoin/pull/25284 , but I think it also makes sense standalone.
The basic idea is that serialization type should not be initialized when it is not needed. Same for the serialization version.
So do this here for `AutoFile`. `CAutoFile` remains in places where it is not yet possible.
ACKs for top commit:
laanwj:
Code review ACK facc2fa7b8
fanquake:
ACK facc2fa7b8
Tree-SHA512: d82d024d55af57565ac53d9d1517afafc12b46964effba0332de62a6c77869356fa77f89e6d4834438fff44c45b64fccdf5a1358bfea03e28dfe55013b3c099d
a02f3f19f5 tidy: use misc-unused-using-decls (fanquake)
d6787bc19b refactor: remove unused using directives (fanquake)
3617634324 validation: remove unused using directives (eugene)
Pull request description:
Adds https://clang.llvm.org/extra/clang-tidy/checks/misc/unused-using-decls.html to our clang-tidy.
PR'd after the discussion in #25433 (which it includes).
ACKs for top commit:
jamesob:
Github ACK a02f3f19f5
Tree-SHA512: 2bb937c1cc90006e69054458d845fb54f287567f4309c773a3fc859f260558c32ff51fc1c2ce9b43207426f3547e7ce226c87186103d741d5efcca19cd355253
8d8eeb422e [net processing] Remove CNode::nLocalServices (John Newbery)
5961f8eea1 [net] Return CService from GetLocalAddrForPeer and GetLocalAddress (dergoegge)
d9079fe18d [net processing] Remove CNode::nServices (John Newbery)
7d1c036934 [net processing] Replace fHaveWitness with CanServeWitnesses() (John Newbery)
f65e83d51b [net processing] Remove fClient and m_limited_node (John Newbery)
fc5eb528f7 [tests] Connect peer in outbound_slow_chain_eviction by sending p2p messages (John Newbery)
1f52c47d5c [net processing] Add m_our_services and m_their_services to Peer (John Newbery)
Pull request description:
Another step in #19398. Which services we offer to a peer and which services they offer to us is application layer data and should not be stored on `CNode`.
This is also a prerequisite for adding `PeerManager` unit tests (See #25515).
ACKs for top commit:
MarcoFalke:
ACK 8d8eeb422e🔑
jnewbery:
utACK 8d8eeb422e
mzumsande:
Code Review ACK 8d8eeb422e
Tree-SHA512: e772eb2a0a85db346dd7b453a41011a12756fc7cbfda6a9ef6daa9633b9a47b9770ab3dc02377690f9d02127301c3905ff22905977f758bf90b17a9a35b37523
cb3e9a1e3f Move {Load,Dump}Mempool to kernel namespace (Carl Dong)
aa30676541 Move DEFAULT_PERSIST_MEMPOOL out of libbitcoinkernel (Carl Dong)
06b88ffb8a LoadMempool: Pass in load_path, stop using gArgs (Carl Dong)
b857ac60d9 test/fuzz: Invoke LoadMempool via CChainState (Carl Dong)
b3267258b0 Move FopenFn to fsbridge namespace (Carl Dong)
ae1e8e3756 mempool: Use NodeClock+friends for LoadMempool (Carl Dong)
f9e8e5719f mempool: Improve comments for [GS]etLoadTried (Carl Dong)
813962da0b scripted-diff: Rename m_is_loaded -> m_load_tried (Carl Dong)
413f4bb52b DumpMempool: Pass in dump_path, stop using gArgs (Carl Dong)
bd4407817e DumpMempool: Use std::chrono instead of weird int64_t arthmetics (Carl Dong)
c84390b741 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 cb3e9a1e3f via `git range-diff 7ae032e...cb3e9a1`
MarcoFalke:
ACK cb3e9a1e3f🔒
ryanofsky:
Code review ACK cb3e9a1e3f
Tree-SHA512: 979d7237c3abb5a1dd9b5ad3dbf3b954f906a6d8320ed7b923557f41a4472deccae3e8a6bca0018c8e7a3c4a93afecc502acd1e26756f2054f157f1c0edd939d
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-
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.
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.
2ef5294a5b rpc: add RPCTypeCheck for getblockfrompeer inputs (Jon Atack)
734b9669ff test: add getblockfrompeer coverage of invalid inputs (Jon Atack)
Pull request description:
The new getblockfrompeer RPC lacks test coverage for invalid arguments, and its error messages are not harmonized with the existing RPCs.
Fix all issues.
ACKs for top commit:
brunoerg:
ACK 2ef5294a5b
Tree-SHA512: 454782cf6a44fd0e05483bb152153667ef5c8021358385ddcf89724fbbbd35e187362bdff757e00c99319527bc4c0b20c7187f67241d4585d767a29787142f25
4c9666bd73 Mention `mempoolfullrbf` in policy/mempool-replacements.md (Antoine Riard)
aae66ab43d Update getmempoolinfo RPC with `mempoolfullrbf` (Antoine Riard)
3e27e31727 Introduce `mempoolfullrbf` node setting. (Antoine Riard)
Pull request description:
This is ready for review.
Recent discussions among LN devs have brought back on the surface concerns about the security of multi-party funded transactions against pinnings attacks and other mempool-based nuisances. The lack of full-rbf transaction-relay topology connected to miners open the way to cheap and naive DoS against multi-party funded transactions (e.g coinjoins, dual-funded channels, on-chain DLCs, ...) without solutions introducing an overhead cost or centralization vectors afaik . For more details, see [0].
This PR implements a simple `fullrbf` setting, where the node always allows transaction replacement, ignoring BIP125 opt-in flag. The default value of the setting stays **false**, therefore opt-in replacement is still the default Bitcoin Core replacement policy. Contrary to a previous proposal of mine and listening to feedbacks collected since then [1], I think this new setting simply offers more flexibility in a node transaction-relay policy suiting one's application requirements, without arguing a change of the default behavior.
I [posted](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-June/020557.html) on the ML to invite operators with a bitcoin application sensitive to full-rbf (e.g dual-funded LN channels service providers) or mempool researchers to join a bootstrapped full-rbf activated peers network for experimentation and learning. If people have strong opinions against the existence of such full-rbf transaction-relay network, I'm proposing to express them on the future thread.
[0] https://lists.linuxfoundation.org/pipermail/lightning-dev/2021-May/003033.html
[1] https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-June/019074.html
Follow-up suggestions :
- soft-enable opt-in RBF in the wallet : https://github.com/bitcoin/bitcoin/pull/25353#issuecomment-1154918789
- p2p discovery and additional outbound connection to full-rbf peers : https://github.com/bitcoin/bitcoin/pull/25353#issuecomment-1156044401
- match the code between RPC, wallet and mempool about disregard of inherited signaling : #22698
ACKs for top commit:
instagibbs:
reACK 4c9666bd73
glozow:
ACK 4c9666bd73, a few nits which are non-blocking.
w0xlt:
ACK 4c9666bd73
Tree-SHA512: 9e288bf22e06a9808804e58178444ef1830c3fdd42fd8a7cd7ffb101f8f586e08b000679be407d63ca76a56f7216227b368ff630c81f3fac3243db1a1202ab1c
This change eliminates memory usage spike when compiling with Visual
Studio 2022 (at least in Cirrus CI environment).
Easy to review using
`git diff --color-moved-ws=allow-indentation-change --color-moved=dimmed-zebra`
This change eliminates memory usage spike when compiling with Visual
Studio 2022 (at least in Cirrus CI environment).
Easy to review using
`git diff --color-moved-ws=allow-indentation-change --color-moved=dimmed-zebra`
This change eliminates memory usage spike when compiling with Visual
Studio 2022 (at least in Cirrus CI environment).
Easy to review using
`git diff --color-moved-ws=allow-indentation-change --color-moved=dimmed-zebra`
27c8056885 rpc: Disallow gettxoutsetinfo queries for a specific block with use_index=false (Martin Zumsande)
Pull request description:
In the `gettxoutsetinfo` RPC, if we set `use_index` to false but specify `hash_or_height`, we currently hit a nonfatal error, e.g. `gettxoutsetinfo "muhash" "1" "false"` results in:
```
Internal bug detected: "!pindex || pindex->GetBlockHash() == view->GetBestBlock()"
rpc/blockchain.cpp:836 (GetUTXOStats)
```
The failing check was added in [#24410](664a14ba7c), but the previous behavior, returning the specified height together with data corresponding to the tip's height, was very confusing too in my opinion.
Fix this by disallowing the interaction of `use_index=false` and `hash_or_height` and add a RPC help example with `-named` because users might ask themselves how to use the `use_index` flag witout hitting an error.
An alternative way would be to allow the interaction if the specified `hash_or_height` happens to correspond to the tip (which should then also be applied to the `HASH_SERIALIZED` check before). If reviewers would prefer that, please say so.
ACKs for top commit:
fjahr:
utACK 27c8056885
shaavan:
ACK 27c8056885
Tree-SHA512: 1d81c34eaa48c86134a2cf7193246d5de6bfd819d413c3b3fae9cb9290e0297a336111eeaecede2f0f020b0f9a181d240de0da4493e1b387fe63b8189154442b
e866f0d066 [functional test] submitrawpackage RPC (glozow)
fa076515b0 [rpc] add new submitpackage RPC (glozow)
Pull request description:
It would be nice for LN/wallet/app devs to test out package policy, package RBF, etc., but the only interface to do so right now is through unit tests. This PR adds a `-regtest` only RPC interface so people can test by submitting raw transaction data. It is regtest-only, as it would be unsafe/confusing to create an actual mainnet interface while package relay doesn't exist.
Note that the functional tests are there to ensure the RPC interface is working properly; they aren't for testing policy itself. See src/test/txpackage_tests.cpp.
ACKs for top commit:
t-bast:
Tested ACK against eclair e866f0d066
ariard:
Code Review ACK e866f0d0
instagibbs:
code review ACK e866f0d066
Tree-SHA512: 824a26b10d2240e0fd85e5dd25bf499ee3dd9ba8ef4f522533998fcf767ddded9f001f7a005fe3ab07ec95e696448484e26599803e6034ed2733125c8c376c84
d1684beabe fees: Pass in a filepath instead of referencing gArgs (Carl Dong)
9a3d825c30 init: Remove redundant -*mempool*, -limit* queries (Carl Dong)
6c5c60c412 mempool: Use m_limit for UpdateTransactionsFromBlock (Carl Dong)
9e93b10301 node/ifaces: Use existing MemPoolLimits (Carl Dong)
38af2bcf35 mempoolaccept: Use limits from mempool in constructor (Carl Dong)
9333427014 mempool: Introduce (still-unused) MemPoolLimits (Carl Dong)
716bb5fbd3 scripted-diff: Rename anc/desc size limit vars to indicate SI unit (Carl Dong)
1ecc77321d scripted-diff: Rename DEFAULT_MEMPOOL_EXPIRY to indicate time unit (Carl Dong)
aa9141cd81 mempool: Pass in -mempoolexpiry instead of referencing gArgs (Carl Dong)
51c7a41a5e init: Only determine maxmempool once (Carl Dong)
386c9472c8 mempool: Make GetMinFee() with custom size protected (Carl Dong)
82f00de7a6 mempool: Pass in -maxmempool instead of referencing gArgs (Carl Dong)
f1941e8bfd pool: Add and use MemPoolOptions, ApplyArgsManOptions (Carl Dong)
0199bd35bb fuzz/rbf: Add missing TestingSetup (Carl Dong)
ccbaf546a6 scripted-diff: Rename DEFAULT_MAX_MEMPOOL_SIZE to indicate SI unit (Carl Dong)
fc02f77ca6 ArgsMan: Add Get*Arg functions returning optional (Carl Dong)
Pull request description:
This is part of the `libbitcoinkernel` project: #24303, https://github.com/bitcoin/bitcoin/projects/18
-----
As mentioned in the Stage 1 Step 2 description of [the `libbitcoinkernel` project](https://github.com/bitcoin/bitcoin/issues/24303), `ArgsManager` will not be part of `libbitcoinkernel`. Therefore, it is important that we remove any dependence on `ArgsManager` by code that will be part of `libbitcoinkernel`. This is the first in a series of PRs aiming to achieve this.
This PR removes `CTxMemPool+MempoolAccept`'s dependency on `ArgsManager` by introducing a `CTxMemPool::Options` struct, which is used to specify `CTxMemPool`'s various options at construction time.
These options are:
- `-maxmempool` -> `CTxMemPool::Options::max_size`
- `-mempoolexpiry` -> `CTxMemPool::Options::expiry`
- `-limitancestorcount` -> `CTxMemPool::Options::limits::ancestor_count`
- `-limitancestorsize` -> `CTxMemPool::Options::limits::ancestor_size`
- `-limitdescendantcount` -> `CTxMemPool::Options::limits::descendant_count`
- `-limitdescendantsize` -> `CTxMemPool::Options::limits::descendant_size`
More context can be gleaned from the commit messages. The important commits are:
- 56eb479ded8bfb2ef635bb6f3b484f9d5952c70d "pool: Add and use MemPoolOptions, ApplyArgsManOptions"
- a1e08b70f3068f4e8def1c630d8f50cd54da7832 "mempool: Pass in -maxmempool instead of referencing gArgs"
- 6f4bf3ede5812b374828f08fc728ceded2f10024 "mempool: Pass in -mempoolexpiry instead of referencing gArgs"
- 5958a7fe4806599fc620ee8c1a881ca10fa2dd16 "mempool: Introduce (still-unused) MemPoolLimits"
Reviewers: Help needed in the following commits (see commit messages):
- a1e08b70f3068f4e8def1c630d8f50cd54da7832 "mempool: Pass in -maxmempool instead of referencing gArgs"
- 0695081a797e9a5d7787b78b0f8289dafcc6bff7 "node/ifaces: Use existing MemPoolLimits"
Note to Reviewers: There are perhaps an infinite number of ways to architect `CTxMemPool::Options`, the current one tries to keep it simple, usable, and flexible. I hope we don't spend too much time arguing over the design here since that's not the point. In the case that you're 100% certain that a different design is strictly better than this one in every regard, please show us a fully-implemented branch.
-----
TODO:
- [x] Use the more ergonomic `CTxMemPool::Options` where appropriate
- [x] Doxygen comments for `ApplyArgsManOptions`, `MemPoolOptions`
-----
Questions for Reviewers:
1. Should we use `std::chrono::seconds` for `CTxMemPool::Options::expiry` and `CTxMemPool::m_expiry` instead of an `int64_t`? Something else? (`std::chrono::hours`?)
2. Should I merge `CTxMemPool::Limits` inside `CTxMemPool::Options`?
ACKs for top commit:
MarcoFalke:
ACK d1684beabe🍜
ryanofsky:
Code review ACK d1684beabe. Just minor cleanups since last review, mostly switching to brace initialization
Tree-SHA512: 2c138e52d69f61c263f1c3648f01c801338a8f576762c815f478ef5148b8b2f51e91ded5c1be915e678c0b14f6cfba894b82afec58d999d39a7bb7c914736e0b
- Store the mempool size limit (-maxmempool) in CTxMemPool as a member.
- Remove the requirement to explicitly specify a mempool size limit for
CTxMemPool::GetMinFee(...) and LimitMempoolSize(...), just use the
stored mempool size limit where possible.
- Remove all now-unnecessary instances of:
gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000
The code change in CChainState::GetCoinsCacheSizeState() is correct
since the coinscache should not repurpose "extra" mempool memory
headroom for itself if the mempool doesn't even exist.
fa956e7508 Replace CountSecondsDouble with Ticks<SecondsDouble> (MacroFake)
Pull request description:
Seems odd to have two ways to say exactly the same thing when one is sufficient.
ACKs for top commit:
fanquake:
ACK fa956e7508
shaavan:
ACK fa956e7508
w0xlt:
ACK fa956e7508
Tree-SHA512: b599470e19b693da1ed1102d1e86b08cb03adaddf2048752b6d050fdf86055be117ff0ae10b6953d03e00eaaf7b0cfa350137968b67d6c5b3ca68c5aa50ca6aa
b80de4c505 test: Test signing psbts without explicitly having scripts (Andrew Chow)
a73b56888a wallet: also search taproot pubkeys in FillPSBT (Andrew Chow)
6cff82722f sign: Use sigdata taproot spenddata when signing (Andrew Chow)
5f12fe3f36 psbt: Implement merge for Taproot fields (Andrew Chow)
1ece9a3715 psbt, test: Check for taproot fields in taproot psbt test (Andrew Chow)
496a1bbe5e taproot: Use pre-existing signatures if available (Andrew Chow)
0ad21e7c55 tests: Test taproot fields for PSBT (Andrew Chow)
103c6fd279 psbt: Remove non_witness_utxo for segwit v1+ (Andrew Chow)
7dccdd3157 Implement decodepsbt for Taproot fields (Andrew Chow)
ac7747585f Fill PSBT Taproot output data to/from SignatureData (Andrew Chow)
25b6ae46e7 Assert that TaprootBuilder is Finalized during GetSpendData (Andrew Chow)
3ae5b6af21 Store TaprootBuilder in SigningProviders instead of TaprootSpendData (Andrew Chow)
4d1223e512 Fetch key origins for Taproot keys (Andrew Chow)
52e3f2f88e Fill PSBT Taproot input data to/from SignatureData (Andrew Chow)
05e2cc9a30 Implement de/ser of PSBT's Taproot fields (Andrew Chow)
d557eff2ad Add serialization methods to XOnlyPubKey (Andrew Chow)
d43923c381 Add TaprootBuilder::GetTreeTuples (Andrew Chow)
ce911204e4 Move individual KeyOriginInfo de/ser to separate function (Andrew Chow)
Pull request description:
Implements the Taproot fields for PSBT described in [BIP 371](https://github.com/bitcoin/bips/blob/master/bip-0371.mediawiki).
ACKs for top commit:
laanwj:
Code review ACK b80de4c505
Tree-SHA512: 50b79bb44f353c9ec2ef4c98aac08a81eba560987e5264a5684caa370e9c4e7a8255c06747fc47749511be45b32d01492e015f92b82be8d22bc8bf192073bd26
baf4efe02f rpc: use enum instead of string for filter type (w0xlt)
Pull request description:
This PR changes the `getblockfilter` RPC to use `BlockFilterType` enum instead of a repeated string for `filtertype_name`.
ACKs for top commit:
furszy:
ACK baf4efe0
brunoerg:
ACK baf4efe02f
Tree-SHA512: 31c79c0a5f0b17fd69b399bb026f523003b656733d6b7d5ffe665921a8cc0f1e0334d2e465145cd89fbd85e196059cf56f4f11563bbc92948b0606080ca76524
fafee78188 rpc: Return incrementalrelayfee in getmempoolinfo (MacroFake)
Pull request description:
Seems odd to return other policy info, but not the incremental relay fee
ACKs for top commit:
1440000bytes:
ACK fafee78188
w0xlt:
Code Review ACK fafee78188
jarolrod:
tACK fafee78188
Tree-SHA512: faad0af6c039b8257acbeac913bc5dcdb2ea2db304c95e52601536c8de60eb1186e9fbb4a64a68adf476605f18022aeda16a5644a0d7912592b0977e4c029638
It could be unsafe/confusing to create an actual mainnet interface while
package relay doesn't exist. However, a regtest-only interface allows
wallet/application devs to test current package policies.
Better to be explicit when it comes to sizes to avoid unintentional
bugs. We use MB and KB all over the place.
-BEGIN VERIFY SCRIPT-
find_regex="DEFAULT_MAX_MEMPOOL_SIZE" \
&& git grep -l -E "$find_regex" \
| xargs sed -i -E "s@$find_regex@\0_MB@g"
-END VERIFY SCRIPT-
0f1a259657 miner: Make mempool optional for BlockAssembler (Carl Dong)
cc5739b27d miner: Make UpdatePackagesForAdded static (Carl Dong)
f024578b3a miner: Absorb SkipMapTxEntry into addPackageTxs (Carl Dong)
Pull request description:
This is part of the libbitcoinkernel project: #24303, https://github.com/bitcoin/bitcoin/projects/18
This is **_NOT_** dependent on, but is a "companion-PR" to #25215.
### Abstract
This PR removes the need to construct `BlockAssembler` with temporary, empty mempools in cases where we don't want to source transactions from the mempool (e.g. in `TestChain100Setup::CreateBlock` and `generateblock`). After this PR, `BlockAssembler` will accept a `CTxMemPool` pointer and handle the `nullptr` case instead of requiring a `CTxMemPool` reference.
An overview of the changes is best seen in the changes in the header file:
```diff
diff --git a/src/node/miner.h b/src/node/miner.h
index 7cf8e3fb9e..7e9f503602 100644
--- a/src/node/miner.h
+++ b/src/node/miner.h
@@ -147,7 +147,7 @@ private:
int64_t m_lock_time_cutoff;
const CChainParams& chainparams;
- const CTxMemPool& m_mempool;
+ const CTxMemPool* m_mempool;
CChainState& m_chainstate;
public:
@@ -157,8 +157,8 @@ public:
CFeeRate blockMinFeeRate;
};
- explicit BlockAssembler(CChainState& chainstate, const CTxMemPool& mempool);
- explicit BlockAssembler(CChainState& chainstate, const CTxMemPool& mempool, const Options& options);
+ explicit BlockAssembler(CChainState& chainstate, const CTxMemPool* mempool);
+ explicit BlockAssembler(CChainState& chainstate, const CTxMemPool* mempool, const Options& options);
/** Construct a new block template with coinbase to scriptPubKeyIn */
std::unique_ptr<CBlockTemplate> CreateNewBlock(const CScript& scriptPubKeyIn);
@@ -177,7 +177,7 @@ private:
/** Add transactions based on feerate including unconfirmed ancestors
* Increments nPackagesSelected / nDescendantsUpdated with corresponding
* statistics from the package selection (for logging statistics). */
- void addPackageTxs(int& nPackagesSelected, int& nDescendantsUpdated) EXCLUSIVE_LOCKS_REQUIRED(m_mempool.cs);
+ void addPackageTxs(const CTxMemPool& mempool, int& nPackagesSelected, int& nDescendantsUpdated) EXCLUSIVE_LOCKS_REQUIRED(mempool.cs);
// helper functions for addPackageTxs()
/** Remove confirmed (inBlock) entries from given set */
@@ -189,15 +189,8 @@ private:
* These checks should always succeed, and they're here
* only as an extra check in case of suboptimal node configuration */
bool TestPackageTransactions(const CTxMemPool::setEntries& package) const;
- /** Return true if given transaction from mapTx has already been evaluated,
- * or if the transaction's cached data in mapTx is incorrect. */
- bool SkipMapTxEntry(CTxMemPool::txiter it, indexed_modified_transaction_set& mapModifiedTx, CTxMemPool::setEntries& failedTx) EXCLUSIVE_LOCKS_REQUIRED(m_mempool.cs);
/** Sort the package in an order that is valid to appear in a block */
void SortForBlock(const CTxMemPool::setEntries& package, std::vector<CTxMemPool::txiter>& sortedEntries);
- /** Add descendants of given transactions to mapModifiedTx with ancestor
- * state updated assuming given transactions are inBlock. Returns number
- * of updated descendants. */
- int UpdatePackagesForAdded(const CTxMemPool::setEntries& alreadyAdded, indexed_modified_transaction_set& mapModifiedTx) EXCLUSIVE_LOCKS_REQUIRED(m_mempool.cs);
};
int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev);
```
### Alternatives
Aside from approach in this current PR, we can also take the approach of moving the `CTxMemPool*` argument from the `BlockAssembler` constructor to `BlockAssembler::CreateNewBlock`, since that's where it's needed anyway. I did not push this approach because it requires quite a lot of call sites to be changed. However, I do have it coded up and can do that if people express a strong preference. This would look something like:
```
BlockAssembler::BlockAssembler(CChainState& chainstate, const Options& options);
BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn, const CTxMemPool* maybe_mempool);
```
### Future work
Although wholly out of scope for this PR, we could potentially refine the `BlockAssembler` interface further, so that we have:
```
BlockAssembler::BlockAssembler(CChainState& chainstate, const Options& options);
BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn, std::vector<CTransaction>& txs);
BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn, const CTxMemPool& mempool);
```
Whereby `TestChain100Setup::CreateBlock` and `generateblock` would call the `BlockAssembler::CreateNewBlock` that takes in `CTransaction`s and we can potentially remove `RegenerateCommitments` altogether. All other callers can use the `CTxMemPool` version.
ACKs for top commit:
glozow:
ACK 0f1a259657
laanwj:
Code review ACK 0f1a259657
MarcoFalke:
ACK 0f1a259657🐊
Tree-SHA512: 2b4b1dbb43d85719f241ad1f19ceb7fc50cf764721da425a3d1ff71bd16328c4f86acff22e565bc9abee770d3ac8827a6676b66daa93dbf42dd817ad929e9448
ce893c0497 doc: Update developer notes (Anthony Towns)
d2852917ee sync.h: Imply negative assertions when calling LOCK (Anthony Towns)
bba87c0553 scripted-diff: Convert global Mutexes to GlobalMutexes (Anthony Towns)
a559509a0b sync.h: Add GlobalMutex type (Anthony Towns)
be6aa72f9f qt/clientmodel: thread safety annotation for m_cached_tip_mutex (Anthony Towns)
f24bd45b37 net_processing: thread safety annotation for m_tx_relay_mutex (Anthony Towns)
Pull request description:
This changes `LOCK(mutex)` for non-global, non-recursive mutexes to be annotated with the negative capability for the mutex it refers to, to prevent . clang applies negative capabilities recursively, so this helps avoid forgetting to annotate functions.
This can't reasonably be used for globals, because clang would require every function to be annotated with `EXCLUSIVE_LOCKS_REQUIRED(!g_mutex)` for each global mutex; so this introduces a trivial `GlobalMutex` subclass of `Mutex`, and reduces the annotations for both `GlobalMutex` to `LOCKS_EXCLUDED` which only catches trivial errors (eg (`LOCK(x); LOCK(x);`).
ACKs for top commit:
MarcoFalke:
review ACK ce893c0497🐦
hebasto:
ACK ce893c0497
Tree-SHA512: 5c35e8c7677ce3d994a7e3774f4344adad496223a51b3a1d1d3b5f20684b2e1d5cff688eb3fbc8d33e1b9940dfa76e515f9434e21de6f3ce3c935e29a319f529
fa4068b4e2 Move minRelayTxFee to policy/settings (MacroFake)
Pull request description:
Seems a bit confusing to put policy stuff into validation, so fix that.
Also fix includes via `iwyu`.
ACKs for top commit:
ariard:
ACK fa4068b, the includes move compiles well locally.
ryanofsky:
Code review ACK fa4068b4e2. Make sense to move the global variable to policy/settings and the default constant to policy/policy. Ariard points out other constants that could be moved, which seems fine, but it seems like moving the global variable to be with other related global variables is more significant.
Tree-SHA512: adf9619002610d1877f3aef0a9e6115fc4c2ad64135a3e5100824c650b560c47f47ac28894c6214a50a7888355252a9f6f7cec98c23a771a1964160ef1ca77de
e593ae07c4 Bugfix: RPC/blockchain: pruneblockchain: Return the height of the actual last pruned block (Luke Dashjr)
Pull request description:
From 0.14 (2017 Mar) until before 0.19 (2019 Nov), the height of the last block pruned was returned, subject to a bug if there were blocks left unpruned due to sharing files with later blocks.
In #15991, this was "fixed" to the current implementation, introducing a new bug: now, it returns the first *unpruned* block.
Since the user provides the parameter as a block to include in pruning, it makes more sense to fix the behaviour to match the documentation.
~~(Additionally, the description of "pruneheight" in getblockchaininfo is fixed to be technically correct)~~
ACKs for top commit:
fjahr:
utACK e593ae07c4
ryanofsky:
Code review ACK e593ae07c4. Just rebased since last review. Maybe some of the original reviewers of #15991 will want to take a look at this to correct the mistake that was introduced there!
Tree-SHA512: c2d511df80682d57260aae8af1665f9d7eaed16448f185f4c9f23c78fa9b8289a02053da7a0b83643fef57610d601ea63b59ff39661a51f4827f1eb27cc30594
...also adjust callers
Changes:
- In BlockAssembler::CreateNewBlock, we now only lock m_mempool->cs and
call addPackageTxs if m_mempool is not nullptr
- BlockAssembler::addPackageTxs now takes in a mempool reference, and is
annotated to require that mempool's lock.
- In TestChain100Setup::CreateBlock and generateblock, don't construct
an empty mempool, just pass in a nullptr for mempool
3a9b9bb38e test: ensure createmultisig and addmultisigaddress are not returning any warning for expected cases (brunoerg)
eaf6f630c0 rpc: fix inappropriate warning for address type p2sh-segwit in createmultisig and addmultisigaddress (brunoerg)
Pull request description:
Fixes #25127
If there are any uncompressed keys when calling `AddAndGetMultisigDestination`, it will just default to a legacy address regardless of the chosen `address_type`. So, #23113 added a warnings field which will warn the user why their address format is different.
However, when creating a multisig (p2sh-segwit), it is returning an inappropriate warning, because when getting the output type from destination (`OutputTypeFromDestination`), it returns `ScriptHash` for both legacy and `P2SH_SEGWIT`. So, since `P2SH_SEGWIT` is different from `ScriptHash`, it returns the warning:
192d639a6b/src/rpc/output_script.cpp (L166-L169)
So, to avoid this mistake I changed `OutputTypeFromDestination` to `descriptor->GetOutputType()` to get the appropriate output type.
ACKs for top commit:
jonatack:
ACK 3a9b9bb38e
laanwj:
Code review ACK 3a9b9bb38e
Tree-SHA512: 49f717479c2b8906277e7591ddd4747f7961c2d5c77494b5124045de9036a4277d46b9ad99279d51f0c4484284c445f1e1d3c55c49bbf0716741bad426a89369
