- Store the mempool expiry (-mempoolexpiry) in CTxMemPool as a
std::chrono::seconds member.
- Remove the requirement to explicitly specify a mempool expiry for
LimitMempoolSize(...), just use the newly-introduced member.
- Remove all now-unnecessary instances of:
std::chrono::hours{gArgs.GetIntArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY)}
ce893c0497fc9b8ab9752153dfcc77c9f427545e doc: Update developer notes (Anthony Towns)
d2852917eecad6ab422a7b2c9892d351a7f0cc96 sync.h: Imply negative assertions when calling LOCK (Anthony Towns)
bba87c0553780eacf0317fbfec7330ea27aa02f8 scripted-diff: Convert global Mutexes to GlobalMutexes (Anthony Towns)
a559509a0b8cade27199740212d7b589f71a0e3b sync.h: Add GlobalMutex type (Anthony Towns)
be6aa72f9f8d50b6b5b19b319a74abe7ab4099ff qt/clientmodel: thread safety annotation for m_cached_tip_mutex (Anthony Towns)
f24bd45b37e1b2d19e5a053dbfefa30306c1d41a 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 ce893c0497fc9b8ab9752153dfcc77c9f427545e 🐦
hebasto:
ACK ce893c0497fc9b8ab9752153dfcc77c9f427545e
Tree-SHA512: 5c35e8c7677ce3d994a7e3774f4344adad496223a51b3a1d1d3b5f20684b2e1d5cff688eb3fbc8d33e1b9940dfa76e515f9434e21de6f3ce3c935e29a319f529
We want m_chainparams to be alive for the duration of
ChainstateManager's lifetime since ChainstateManager's behaviour depends
on m_chainparams.
We could allow for a std::shared_ptr to be passed in as m_chainparams,
but that complicates things further. Given that CChainParams is not an
entity class or struct, we can just copy it and have ChainstateManager
own it.
This decouples validation.cpp from netaddress.cpp (transitively,
timedata.cpp, and asmap.cpp).
This is important for libbitcoinkernel as:
- There is no reason for the consensus engine to be coupled with
netaddress, timedata, and asmap
- Users of libbitcoinkernel can now easily supply their own
std::function that provides the adjusted time.
See the src/Makefile.am changes for some satisfying removals.
[META] Although it seems like we don't need it for just one option,
we're going to introduce another member to this struct *in the
next commit*. In future patchsets for libbitcoinkernel decoupling
it from ArgsManager, even more members will be added here.
f64aa9c411ad78259756a28756ec1eb8069b5ab4 Disallow more unsafe string->path conversions allowed by path append operators (Ryan Ofsky)
Pull request description:
Add more `fs::path` `operator/` and `operator+` overloads to prevent unsafe string->path conversions on Windows that would cause strings to be decoded according to the current Windows locale & code page instead of the correct string encoding.
Update application code to deal with loss of implicit string->path conversions by calling `fs::u8path` or `fs::PathFromString` explicitly, or by just changing variable types from `std::string` to `fs::path` to avoid conversions altogether, or make them happen earlier.
In all cases, there's no change in behavior either (1) because strings only contained ASCII characters and would be decoded the same regardless of what encoding was used, or (2) because of the 1:1 mapping between paths and strings using the `PathToString` and `PathFromString` functions.
Motivation for this PR was just that I was experimenting with #24469 and noticed that operations like `fs::path / std::string` were allowed, and I thought it would be better not to allow them.
ACKs for top commit:
hebasto:
ACK f64aa9c411ad78259756a28756ec1eb8069b5ab4
Tree-SHA512: 944cce49ed51537ee7a35ea4ea7f5feaf0c8fff2fa67ee81ec5adebfd3dcbaf41b73eb35e49973d5f852620367f13506fd12a7a9b5ae3a7a0007414d5c9df50f
In previous commits in this patchset, we've made sure that every
Unload/UnloadBlockIndex member function resets its own members, and does
not reach out to globals.
This means that their corresponding classes' default destructors can now
replace them, and do an even more thorough job without the need to be
updated for every new member variable.
Therefore, we can remove them, and also remove UnloadBlockIndex since
that's not used anymore.
Unfortunately, chainstatemanager_loadblockindex relies on
CChainState::UnloadBlockIndex, so that needs to stay for now.
fa1970f075292d7312654730a994a68c2ca8bc06 Make BlockManager::LoadBlockIndex private (MarcoFalke)
Pull request description:
* After commit fa27f03b4943540aa2eab283d4cf50ad4a1a01f8 `BlockManager::LoadBlockIndex` is only called by `BlockManager::LoadBlockIndexDB`. Thus, it can be made `private`.
* After commit c600ee38168a460d3026eae0e289c976194aad14 `m_best_invalid` is no longer accessed by `BlockManager::LoadBlockIndex`. Thus, the unused `friend` can be removed.
ACKs for top commit:
mruddy:
ACK fa1970f075292d7312654730a994a68c2ca8bc06 I verified by double checking references, then applying the patch, and running `make check`. LGTM.
Tree-SHA512: 9b36b4c59bf7ad01171764ce61b1be9750fc92d105c4fe939b1a6a70027ab6300d5d2a2fc3e82f981e22c3987f2ca84e092d2e1f8463fa320af9f05048580c0a
Add more fs::path operator/ and operator+ overloads to prevent unsafe
string->path conversions on Windows that would cause strings to be
decoded according to the current Windows locale & code page instead of
the correct string encoding.
Update application code to deal with loss of implicit string->path
conversions by calling fs::u8path or fs::PathFromString explicitly, or
by just changing variable types from std::string to fs::path to avoid
conversions altoghther, or make them happen earlier.
In all cases, there's no change in behavior either (1) because strings
only contained ASCII characters and would be decoded the same regardless
of what encoding was used, or (2) because of the 1:1 mapping between
paths and strings using the PathToString and PathFromString functions.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
f865cf8ded2b2fbc82a6fbc41226d991909a6880 Add and use BlockManager::GetAllBlockIndices (Carl Dong)
28ba0313eac37e4a900b7e97af7169ce999c4024 Add and use CBlockIndexHeightOnlyComparator (Carl Dong)
12eb05df63f930969115af6dc66e2e5d02f2a517 move-only: Move CBlockIndexWorkComparator to blockstorage (Carl Dong)
c600ee38168a460d3026eae0e289c976194aad14 Only load BlockMan in BlockMan member functions (Carl Dong)
42e56d9b188f97c077ed2269e24acc0be35ece17 style-only: No need for std::pair for vSortedByHeight (Carl Dong)
3bbb6fea051f4e19bd2448e401a6c4e9b4cc7a41 style-only: Various blockstorage.cpp cleanups (Carl Dong)
5be9ee3c54dcb396ff52fc8c8b7e4e6e39ec4a3b refactor: more const annotations for uses of CBlockIndex* (Anthony Towns)
Pull request description:
The only important commit is "Only load BlockMan in BlockMan member functions", everything else is all just small style changes.
Here's the commit message, reproduced:
```
This commit effectively splits the "load block index itself" logic from
"derive Chainstate variables from loaded block index" logic.
This means that BlockManager::LoadBlockIndex{,DB} will only load what's
relevant to the BlockManager.
```
ACKs for top commit:
ajtowns:
ACK f865cf8ded2b2fbc82a6fbc41226d991909a6880 ; code review only
MarcoFalke:
review ACK f865cf8ded2b2fbc82a6fbc41226d991909a6880 🗂
Tree-SHA512: 7b204d782834e06fd7329d022e2ae860181b4e8105c33bfb928539a4ec24161dc7438a9c4d4ee279dcad77de310c160b997bb8aa18923243d0fd55ccf4ad7c3a
This commit effectively splits the "load block index itself" logic from
"derive Chainstate variables from loaded block index" logic.
This means that BlockManager::LoadBlockIndex{,DB} will only load what's
relevant to the BlockManager.
I strongly recommend reviewing with the following git-diff flags:
--color-moved=dimmed_zebra --color-moved-ws=allow-indentation-change
fa8d4d9128c35de0fe715f2e2b99269d23c09cc1 scripted-diff: Clarify CheckFinalTxAtTip name (MarcoFalke)
fa4e30b0f36f2e7a09db7d30dca9008ed9dbcb35 policy: Remove unused locktime flags (MarcoFalke)
Pull request description:
The locktime flags have many issues:
* They are passed in by a default argument, which is fragile. It has already lead to bugs like the one fixed in commit e30b6ea194fee3bb95a45e7b732a99566b88f1f5.
* They are negative (signed), which doesn't make sense for flags (unsigned in general). According to the review comments when the code was added: "The max on the flags is a fairly weird operation." (https://github.com/bitcoin/bitcoin/pull/6566#issuecomment-150310861)
* No call site relies on the default argument and they all pass in a single compile-time constant, rendering most of the code dead and untested.
* The dead code calls `GetAdjustedTime` (network adjusted time), which has its own issues. See https://github.com/bitcoin/bitcoin/issues/4521
Fix all issues by removing them
ACKs for top commit:
ajtowns:
ACK fa8d4d9128c35de0fe715f2e2b99269d23c09cc1
theStack:
Code-review ACK fa8d4d9128c35de0fe715f2e2b99269d23c09cc1
glozow:
ACK fa8d4d9128c35de0fe715f2e2b99269d23c09cc1, agree the default arg `flags` is a massive footgun and just setting max flags is weird. Adding `AtTip` to the names makes sense to me, since they're both testing for *next* block and only ever used for {,re}addition to mempool.
Tree-SHA512: 79f4a52f34909eb598d88bbae7afe8abe5f85f45c128483d16aa83dacd0e5579e561b725d01b1e9a931d1821012a51ad2bc6fb2867f8d09ee541f9d234d696f8
ae9ceed3e23288b163b7d7b1840b06b8d332f4ce validation, refactoring: remove ChainstateManager::Reset() (Jon Atack)
daad0093e3d1466789d0ce687902636c80cd74a1 validation: replace lock with annotation in UnloadBlockIndex() (Jon Atack)
Pull request description:
Thread safety refactoring seen in #24177:
- replace re-acquiring lock cs_main with a thread safety annotation in UnloadBlockIndex()
- remove ChainstateManager::Reset(), as it is currently unused (can be reintroduced in the test utilities if needed for unit testing)
ACKs for top commit:
laanwj:
Code review ACK ae9ceed3e23288b163b7d7b1840b06b8d332f4ce
vasild:
ACK ae9ceed3e23288b163b7d7b1840b06b8d332f4ce
klementtan:
crACK ae9ceed3e23288b163b7d7b1840b06b8d332f4ce
Tree-SHA512: cebb782572997cc2dda01590d6bb6c5e479e8202324d8b6ff459b814ce09e818b996c881736bfebd1b8bf4b6d7a0f79faf3ffea176a4699dd7d7429de2db2d13
fa8dad0e078c577d740a9667636733957586c035 rpc: Fix implicit-integer-sign-change in verifychain (MarcoFalke)
Pull request description:
It doesn't really make sense to treat `DEFAULT_CHECKLEVEL` as unsigned as long as `VerifyDB` accepts a signed integer.
Making it signed also avoids a cast round trip from signed->unsigned->signed in the RPC.
ACKs for top commit:
luke-jr:
utACK fa8dad0e078c577d740a9667636733957586c035
theStack:
Code-review ACK fa8dad0e078c577d740a9667636733957586c035
Tree-SHA512: 75499dbe4ace2962792e5fbec7defb10c25fdbbfde951d5e542a91daa880cc50395da0287173e2c84a28e18267c74af7b44b9f38ce364bcb0216c402f65b7641
020acea99b605c9b5ee7939a6acef131db84ad4a refactor: replace RecursiveMutex m_chainstate_mutex with Mutex (w0xlt)
ddeefeef20fa2fe48c3c4563370a6297704d228e refactor: add negative TS annotations for `m_chainstate_mutex` (w0xlt)
1dfd31bc267c54144a7e62ad5a1a5860c032f4d7 scripted-diff: rename m_cs_chainstate -> m_chainstate_mutex (w0xlt)
Pull request description:
This PR is related to #19303 and gets rid of the `RecursiveMutex m_cs_chainstate`.
`m_cs_chainstate` is only held in `ActivateBestChain()` and `InvalidateBlock()`.
So apparently there is no recursion involved, so the `m_cs_chainstate` can be a non-recursive mutex.
ACKs for top commit:
hebasto:
ACK 020acea99b605c9b5ee7939a6acef131db84ad4a, I have reviewed the code and it looks OK, I agree it can be merged.
theStack:
Code-review ACK 020acea99b605c9b5ee7939a6acef131db84ad4a 🌴
shaavan:
reACK 020acea99b605c9b5ee7939a6acef131db84ad4a
Tree-SHA512: c7c16e727e326df3410514915ce753a2a5e1da78857ef965ef683e36251e1b73c9cced4cd5231b04dbe2be0ea14084f6731b4d7a4d9a8e086e982b985e37e4b4
This checks finality at the current Tip, so clarify this in its name.
-BEGIN VERIFY SCRIPT-
ren() { sed -i "s/\<$1\>/$2/g" $( git grep -l "$1" ./src/ ) ; }
ren CheckSequenceLocks CheckSequenceLocksAtTip
ren CheckFinalTx CheckFinalTxAtTip
-END VERIFY SCRIPT-
3cd7f693d3ed1bb7cf9ba3e0c482174df3684972 [unit test] package parents are a mix (glozow)
de075a98eaf0b3f7676c5c78b50b66902202b34c [validation] better handle errors in SubmitPackage (glozow)
9d88853e0c85f765f7d982b15e8122ede50110ed AcceptPackage fixups (glozow)
2db77cd3b835d052de678755bcdde5a645ce2d65 [unit test] different witness in package submission (glozow)
9ad211c5753dbd148ba6f0ed56854f6364362ca8 [doc] more detailed explanation for deduplication (glozow)
83d4fb71260f268abd41d083fb3458476aed83ce [packages] return DIFFERENT_WITNESS for same-txid-different-witness tx (glozow)
Pull request description:
This addresses some comments from review on e12fafda2dfbbdf63f125e5af797ecfaa6488f66 from #22674.
- Improve documentation about de-duplication: [comment](https://github.com/bitcoin/bitcoin/pull/22674/files#r770156708)
- Fix code looking up same-txid-different-witness transaction in mempool: [comment](https://github.com/bitcoin/bitcoin/pull/22674/files#r770804029)
- Improve the interface for when a same-txid-different-witness transaction is swapped: [comment](https://github.com/bitcoin/bitcoin/pull/22674/files#r770782822)
- Add a test for witness swapping: [comment](https://github.com/bitcoin/bitcoin/pull/22674/files#r770804029)
- Add a test for packages with a mix of duplicate/different witness/new parents: [comment](https://github.com/bitcoin/bitcoin/pull/22674#discussion_r773037608)
- Fix issue with not notifying `CValidationInterface` when there's a partial submission due to fail-fast: [comment](https://github.com/bitcoin/bitcoin/pull/22674#discussion_r773013162)
ACKs for top commit:
achow101:
ACK 3cd7f693d3ed1bb7cf9ba3e0c482174df3684972
t-bast:
LGTM, ACK 3cd7f693d3
instagibbs:
ACK 3cd7f693d3ed1bb7cf9ba3e0c482174df3684972
ariard:
ACK 3cd7f69
Tree-SHA512: a5d86ca86edab80a5a05fcbb828901c058b3f2fa2552912ea52f2871e29c3cf4cc34020e7aac2217959c9c3a01856f4bd3d631d844635b98144f212f76c2f3ef
The previous interface required callers to guess that the tx had been
swapped and look up the tx again by txid to find a `MEMPOOL_ENTRY`
result. This is a confusing interface.
Instead, explicitly tell the caller that this transaction was
`DIFFERENT_WITNESS` in the result linked to the mempool entry's wtxid.
This gives the caller all the information they need in 1 lookup, and
they can query the mempool for the other transaction if needed.
BlockManager is a large data structure, and cs_main is not required to
take its address or access every part of it. Individual BlockManager
fields and methods which do require cs_main like m_block_index and
LookupBlockIndex are already annotated separately, and these other
annotations describe locking requirements more accurately and do a
better job enforcing thread safety.
Since cs_main is not needed to access the address of the m_block object,
this commit drops cs_main LOCK calls which were added pointlessly to
satisfy this annotation in the past.
Co-authored-by: Carl Dong <contact@carldong.me>
