48742693acc9de837735674057c9aae2fe90bd1d Replace "can not" with "cannot" in docs, user messages, and tests (Jon Atack)
e670edd43441ecb6e5978d65348501c57d856030 User-facing content fixups from transifex translator feedback (Jon Atack)
Pull request description:
Closes#24366.
ACKs for top commit:
laanwj:
Code review re-ACK 48742693acc9de837735674057c9aae2fe90bd1d
hebasto:
re-ACK 48742693acc9de837735674057c9aae2fe90bd1d, only suggested change since my previous [review](https://github.com/bitcoin/bitcoin/pull/24367#pullrequestreview-885938219).
Tree-SHA512: 4dcdcb417251a413e65fab6070515e13a1267c8e0dbcf521386b842511391f24c84a0c2168fe13458c977682034466509bf2a3453719d4d94d3c568fd9f4adb4
77202f0554dcbbbb167d0ed3927cca0bf4609ce8 [doc] package deduplication (glozow)
d35a3cb3968d7584c7d5c42b121a80f34ea656bf [doc] clarify inaccurate comment about replacements paying higher feerate (glozow)
5ae187f8761f5f85a1ef41d24f75afb7eecf366f [validation] look up transaction by txid (glozow)
Pull request description:
- Use txid, not wtxid, for `mempool.GetIter()`: https://github.com/bitcoin/bitcoin/pull/22674#discussion_r772934994
- Fix a historically inaccurate comment about RBF during the refactors: https://github.com/bitcoin/bitcoin/pull/22855#discussion_r777130441
- Add a section about package deduplication to policy/packages.md: https://github.com/bitcoin/bitcoin/pull/24152#discussion_r802955759 and https://github.com/bitcoin/bitcoin/pull/24152#discussion_r802723149
(I'm intending for this to be in v23 since it's fixups for things that are already merged, which is why I split it from #24152)
ACKs for top commit:
t-bast:
LGTM, ACK 77202f0554
darosior:
ACK 77202f0554dcbbbb167d0ed3927cca0bf4609ce8
LarryRuane:
ACK 77202f0554dcbbbb167d0ed3927cca0bf4609ce8
Tree-SHA512: a428e791dfa59c359d3ccc67e8d3a4c1239815d2f6b29898e129700079271c00b3a45f091f70b65a6e54aa00a3d5b678b6da29d2a76b6cd6f946eaa7082ea696
799968e8b38833dc7fd7b6d488a66a14580ef674 tracing: misc follow-ups to 22902 (0xb10c)
36a65847033540cf2203252c7baf42bc5ec97579 tracing: correctly scope utxocache:flush tracepoint (Arnab Sen)
Pull request description:
This PR is a follow-up to the [#22902](https://github.com/bitcoin/bitcoin/pull/22902).
Previously, the tracepoint `utxocache:flush` was called, even when it was not flushing. So, the tracepoint is now scoped to write only when coins cache to disk.
ACKs for top commit:
0xB10C:
ACK 799968e8b38833dc7fd7b6d488a66a14580ef674
Tree-SHA512: ebb096cbf991c551c81e4339821f10d9768c14cf3d8cb14d0ad851acff5980962228a1c746914c6aba3bdb27e8be53b33349c41efe8bab5542f639916e437b5f
eb8b22d5176d7abc6f93b4473df446105ca595e6 block_connected: re-use previous GetTimeMicros (William Casarin)
80e1c55687aae61767f1ade0826746cda00d6a24 block_connected: don't serialize block hash twice (William Casarin)
Pull request description:
In the validation:block_connected tracepoint, we call block->GetHash(), which
ends up calling CBlockHeader::GetHash(), executing around 8000 serialization
instructions. We don't need to do this extra work, because block->GetHash() is
already called further up in the function. Let's save that value as a local
variable and re-use it in our tracepoint so there is no unnecessary tracepoint
overhead.
Shave off an extra 100 or so instructions from the validation:block_connected
tracepoint by reusing a nearby GetTimeMicros(). This brings the tracepoint down
to 54 instructions. Still high, but much better than the previous ~154 and
8000 instructions which it was originally.
Signed-off-by: William Casarin <jb55@jb55.com>
ACKs for top commit:
0xB10C:
ACK eb8b22d5176d7abc6f93b4473df446105ca595e6
laanwj:
Code review ACK eb8b22d5176d7abc6f93b4473df446105ca595e6
theStack:
re-ACK eb8b22d5176d7abc6f93b4473df446105ca595e6
Tree-SHA512: 92ae585e487554e0f73042a8abaa239f630502c1d198e010bd7c1de252d882bccb627bbf0e4faec09c1253e782b145bcf153f9fee78cdb8456188044a96f8267
f485a0745455b46390f1c14260643ad69c8fe2ad Add missing thread safety lock assertions in validation.h (Jon Atack)
37af8a20cf39ed8ee4b3ba4e1d8d55178eaacb78 Add missing thread safety lock assertions in validation.cpp (Jon Atack)
Pull request description:
A number of functions in validation.{h,cpp} have a thread safety lock annotation in the declaration but are missing the corresponding run-time lock assertion in the definition.
ACKs for top commit:
hebasto:
re-ACK f485a0745455b46390f1c14260643ad69c8fe2ad, only suggested change since my [previous](https://github.com/bitcoin/bitcoin/pull/24177#pullrequestreview-877810465) review.
vasild:
ACK f485a0745455b46390f1c14260643ad69c8fe2ad
Tree-SHA512: c86c0c0e8fe6ec7ae9ed9890f1dd7d042aa482ecf99feb6679a670aa004f6e9a99f7bc047205a34968fab7f1f841898c59b48c3ed6245c166e3b5abbf0867445
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
fa5d2e678c809c26bd40d7e7c171529d3ffb5903 Remove unused char serialize (MarcoFalke)
fa24493d6394b3a477535f480664c9596f18e3c5 Use spans of std::byte in serialize (MarcoFalke)
fa65bbf217b725ada35107b4ad646d250228355c span: Add BytePtr helper (MarcoFalke)
Pull request description:
This changes the serialize code (`.read()` and `.write()` functions) to take a `Span` instead of a pointer and size. This is a breaking change for the serialize interface, so at no additional cost we can also switch to `std::byte` (instead of using `char`).
The benefits of using `Span`:
* Less verbose and less fragile code when passing an already existing `Span`(-like) object to or from serialization
The benefits of using `std::byte`:
* `std::byte` can't accidentally be mistaken for an integer
The goal here is to only change serialize to use spans of `std::byte`. If needed, `AsBytes`, `MakeUCharSpan`, ... can be used (temporarily) to pass spans of the right type.
Other changes that are included here:
* [#22167](https://github.com/bitcoin/bitcoin/pull/22167) (refactor: Remove char serialize by MarcoFalke)
* [#21906](https://github.com/bitcoin/bitcoin/pull/21906) (Preserve const in cast on CTransactionSignatureSerializer by promag)
ACKs for top commit:
laanwj:
Concept and code review ACK fa5d2e678c809c26bd40d7e7c171529d3ffb5903
sipa:
re-utACK fa5d2e678c809c26bd40d7e7c171529d3ffb5903
Tree-SHA512: 08ee9eced5fb777cedae593b11e33660bed9a3e1711a7451a87b835089a96c99ce0632918bb4666a4e859c4d020f88fb50f2dd734216b0c3d1a9a704967ece6f
c5b36b1c1b11f04e5da7fb44183f61d09a14e40d Mempool Update Cut-Through Optimization (Jeremy Rubin)
c49daf9885e86ba08acdc8332d2a34bc5951a487 [TESTS] Increase limitancestorcount in tournament RPC test to showcase improved algorithm (Jeremy Rubin)
Pull request description:
Often when we're updating mempool entries we update entries that we ultimately end up removing the updated entries shortly thereafter. This patch makes it so that we filter for such entries a bit earlier in processing, which yields a mild improvement for these cases, and is negligible overhead otherwise.
There's potential for a better -- but more sophisticated -- algorithm that can be used taking advantage of epochs, but I figured it is better to do something that is simple and works first and upgrade it later as the other epoch mempool work proceeds as it makes the patches for the epoch algorithm simpler to understand, so you can consider this as preparatory work. It could either go in now if it is not controversial, or we could wait until the other patch is ready to go.
ACKs for top commit:
instagibbs:
reACK c5b36b1
sipa:
utACK c5b36b1c1b11f04e5da7fb44183f61d09a14e40d
mzumsande:
Code Review ACK c5b36b1c1b11f04e5da7fb44183f61d09a14e40d
Tree-SHA512: 78b16864f77a637d8a68a65e23c019a9757d8b2243486728ef601d212ae482f6084cf8e69d810958c356f1803178046e4697207ba40d6d10529ca57de647fae6
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
No behavior change.
This code was introduced in 5add7a7 before we required C++11, which is
why the struct was needed. As we are now using more modern C++ and this
is the only place where lockpoints are updated for mempool entries, it
is more idiomatic to call `modify` with a lambda.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Behavior change: don't quit right after LimitMempoolSize() when a
package is partially submitted. We should still send
TransactionAddedToMempool notifications for
transactions that were submitted.
Not behavior change: add a new package validation result for mempool logic errors.
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.
fac22fd36b2d9f55dada31cc0da55520431b972a log: Remove GetAdjustedTime from IBD header progress estimation (MarcoFalke)
Pull request description:
This is a "refactor" that shouldn't change behaviour, because the two times are most likely equal. A minimum of 5 outbound peers are needed to adjust the time. And if the time is adjusted, it will be by at most 70 minutes (`DEFAULT_MAX_TIME_ADJUSTMENT`). Thus, the progress estimate should differ by at most 7 blocks.
ACKs for top commit:
laanwj:
Code review ACK fac22fd36b2d9f55dada31cc0da55520431b972a
vincenzopalazzo:
ACK fac22fd36b
Tree-SHA512: bf9f5eef66db0110dd268cf6dbfab64b9c11ba776924f5b386ceae3f2d005272cceb87ebcc96e0c8b854c051514854a2a5af39ae43bad008fac685b5aafaabd0
Shave off an extra 100 or so instructions from the
validation:block_connected tracepoint by reusing a nearby
GetTimeMicros(). This brings the tracepoint down to 54 instructions.
Still high, but much better than the previous ~154 and 8000 instructions
which it was originally.
Signed-off-by: William Casarin <jb55@jb55.com>
In the validation:block_connected tracepoint, we call block->GetHash(),
which ends up calling CBlockHeader::GetHash(), executing around 8000
serialization instructions. We don't need to do this extra work, because
block->GetHash() is already called further up in the function. Let's
save that value as a local variable and re-use it in our tracepoint so
there is no unnecessary tracepoint overhead.
Signed-off-by: William Casarin <jb55@jb55.com>
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>
This is a refactor and safe to do because:
* UnloadBlockIndex calls ChainstateManager::Unload, which calls
BlockManager::Unload
* Only unit tests call Unload directly
fa7efc915b87ec56ca1cc0bad7d8f79591bfa099 Fixup style of moved code (MarcoFalke)
fade2a44f4aabc64185031dbf4c70d875ece6740 Move BlockManager to node/blockstorage (MarcoFalke)
Pull request description:
`BlockManager` is responsible for reading and writing block(headers). So move it to the existing `blockstorage` module in `node`. Also, move validation code unrelated to block-storage out from `BlockManager`.
ACKs for top commit:
ryanofsky:
Code review obvious ACK fa7efc915b87ec56ca1cc0bad7d8f79591bfa099
Tree-SHA512: 0197943d818e5f59e743b07fbb92e7661bff90081127a41e35e5692ce49d6f6a7872448670b0da282f7714580a45c8d93e571a67177c8b5f785ce9edefe834c5
b4adc5ad6769e4a5a6179dfff271cd4c9dc47a5b [bugfix] update lockpoints correctly during reorg (glozow)
b6002b07a36f0d58dc6becd04bfcf78599056b7c MOVEONLY: update_lock_points to txmempool.h (glozow)
Pull request description:
I introduced a bug in #22677 (sorry! 😅)
Mempool entries cache `LockPoints`, containing the first height/blockhash/`CBlockIndex*` at which the transaction becomes valid. During a reorg, we re-check timelocks on all mempool entries using `CheckSequenceLocks(useExistingLockPoints=false)` and remove any now-invalid entries. `CheckSequenceLocks()` also mutates the `LockPoints` passed in, and we update valid entries' `LockPoints` using `update_lock_points`. Thus, `update_lock_points(lp)` needs to be called right after `CheckSequenceLocks(lp)`, otherwise we lose the data in `lp`. I incorrectly assumed they could be called in separate loops.
The incorrect behavior introduced is: if we have a reorg in which a timelocked mempool transaction is still valid but becomes valid at a different block, the cached `LockPoints` will be incorrect.
This PR fixes the bug, adds a test, and adds an assertion at the end of `removeForReorg()` to check that all mempool entries' lockpoints are valid. You can reproduce the bug by running the test added in the [test] commit on the code before the [bugfix] commit.
ACKs for top commit:
jnewbery:
ACK b4adc5ad6769e4a5a6179dfff271cd4c9dc47a5b
vasild:
ACK b4adc5ad6769e4a5a6179dfff271cd4c9dc47a5b
mzumsande:
Code Review ACK b4adc5ad6769e4a5a6179dfff271cd4c9dc47a5b
hebasto:
ACK b4adc5ad6769e4a5a6179dfff271cd4c9dc47a5b
MarcoFalke:
re-ACK b4adc5ad6769e4a5a6179dfff271cd4c9dc47a5b 🏁
Tree-SHA512: 16b59f6ff8140d0229079ca1c6b04f2f4a00a2e49931275150e4f3fe5ac4ec109698b083fa6b223ba9511f328271cc1ab081263669d5da020af7fee83c13e401
fadd73037e266edb844f0972e82e9213171ef214 refactor: Remove implicit-integer-sign-change suppressions in validation.cpp (MarcoFalke)
Pull request description:
A file-wide suppression is problematic because it will wave through future violations, potentially bugs.
Fix that by using per-statement casts.
ACKs for top commit:
shaavan:
ACK fadd73037e266edb844f0972e82e9213171ef214
theStack:
Code-review ACK fadd73037e266edb844f0972e82e9213171ef214
Tree-SHA512: a8a05613be35382b92d7970f958a4e8f4332432056eaa9d72f6719495134b93aaaeea692899d9035654d0e0cf56bcd759671eeeacfd0535582c0ea048ab58a56
fa1a51cbc1c50a6d3adcad5ccea4c6067f89f7d3 doc: testnet3 was not reset and is doing BIP30 checks again (MarcoFalke)
Pull request description:
ACKs for top commit:
theStack:
ACK fa1a51cbc1c50a6d3adcad5ccea4c6067f89f7d3
Tree-SHA512: 793eccda583a3edb056b142c36a09a5c867f61d90b96e15e6643417d62eb651eb2f3429c5f245bdb062d18ab9bb05b5048c0888aa5a492cb7bb21a2f3f52324e
Previously, the `utxocache:flush` tracepoint was in the wrong scope and
reached every time `CChainState::FlushStateToDisk` was called, even when
there was no flushing of the cache. The tracepoint is now properly scoped
and will be reached during a full flush.
Inside the scope, the `fDoFullFlush` value will always be `true`, so it
doesn't need to be logged separately. Hence, it's dropped from the
tracepoint arguments.
fab6d6b2d154893ab422dda87f3535d42c3e06f4 Move pindexBestInvalid to ChainstateManager (MarcoFalke)
facd2137eceacb95e1f71c87ddc704d752b37272 Move m_failed_blocks to ChainstateManager (MarcoFalke)
fa47b5c100f81c65c15b5a6afaf6c91bc0861264 Move AcceptBlockHeader to ChainstateManager (MarcoFalke)
fa3d62cf7b3501a056b34c5458c14d2fe6a55bd7 Move FindForkInGlobalIndex from BlockManager to CChainState (MarcoFalke)
Pull request description:
Move globals or members of the wrong class to the right class.
ACKs for top commit:
naumenkogs:
ACK fab6d6b2d154893ab422dda87f3535d42c3e06f4
Sjors:
ACK fab6d6b2d154893ab422dda87f3535d42c3e06f4
shaavan:
ACK fab6d6b2d154893ab422dda87f3535d42c3e06f4
Tree-SHA512: 926cbdfa22838517497bacb79ed5f521f64117c2aacf96a0176f62831b4713314a32abc0213df5ee067edf63e4a4300f752a26006d36e5aab415bb91209a271f
The member is unrelated to block storage (BlockManager). It is related
to validation.
Fix the confusion by moving it.
Can be reviewed with
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
