Despite removeUnchecked not explicitly requiring CDeterministicMNManager,
it has also been made conditional due to addUnchecked requiring the manager
and allowing for some operations but not others when pertaining to a
transaction type could allow inconsistencies to arise. Better to treat as
one unit and skip both if manager isn't present.
296be8f58e02b39a58f017c52294aceed22c3ffd Get rid of unused functions CTxMemPool::GetMemPoolChildren, CTxMemPool::GetMemPoolParents (Jeremy Rubin)
46d955d196043cc297834baeebce31ff778dff80 Remove mapLinks in favor of entry inlined structs with iterator type erasure (Jeremy Rubin)
Pull request description:
Currently we have a peculiar data structure in the mempool called maplinks. Maplinks job is to track the in-pool children and parents of each transaction. This PR can be primarily understood and reviewed as a simple refactoring to remove this extra data structure, although it comes with a nice memory and performance improvement for free.
Maplinks is particularly peculiar because removing it is not as simple as just moving it's inner structure to the owning CTxMempoolEntry. Because TxLinks (the class storing the setEntries for parents and children) store txiters to each entry in the mempool corresponding to the parent or child, it means that the TxLinks type is "aware" of the boost multiindex (mapTx) it's coming from, which is in turn, aware of the entry type stored in mapTx. Thus we used maplinks to store this entry associated data we in an entirely separate data structure just to avoid a circular type reference caused by storing a txiter inside a CTxMempoolEntry.
It turns out, we can kill this circular reference by making use of iterator_to multiindex function and std::reference_wrapper. This allows us to get rid of the maplinks data structure and move the ownership of the parents/child sets to the entries themselves.
The benefit of this good all around, for any of the reasons given below the change would be acceptable, and it doesn't make the code harder to reason about or worse in any respect (as far as I can tell, there's no tradeoff).
### Simpler ownership model
No longer having to consistency check that mapLinks did have records for our CTxMempoolEntry, impossible to have a mapLinks entry outlive or incorrectly die before a CTxMempoolEntry.
### Memory Usage
We get rid of a O(Transactions) sized map in the mempool, which is a long lived data structure.
### Performance
If you have a CTxMemPoolEntry, you immediately know the address of it's children/parents, rather than having to do a O(log(Transactions)) lookup via maplinks (which we do very often). We do it in *so many* places that a true benchmark has to look at a full running node, but it is easy enough to show an improvement in this case.
The ComplexMemPool shows a good coherence check that we see the expected result of it being 12.5% faster / 1.14x faster.
```
Before:
# Benchmark, evals, iterations, total, min, max, median
ComplexMemPool, 5, 1, 1.40462, 0.277222, 0.285339, 0.279793
After:
# Benchmark, evals, iterations, total, min, max, median
ComplexMemPool, 5, 1, 1.22586, 0.243831, 0.247076, 0.244596
```
The ComplexMemPool benchmark only checks doing addUnchecked and TrimToSize for 800 transactions. While this bench does a good job of hammering the relevant types of function, it doesn't test everything.
Subbing in 5000 transactions shows a that the advantage isn't completely wiped out by other asymptotic factors (this isn't the only bottleneck in growing the mempool), but it's only a bit proportionally slower (10.8%, 1.12x), which adds evidence that this will be a good change for performance minded users.
```
# Benchmark, evals, iterations, total, min, max, median
ComplexMemPool, 5, 1, 59.1321, 11.5919, 12.235, 11.7068
# Benchmark, evals, iterations, total, min, max, median
ComplexMemPool, 5, 1, 52.1307, 10.2641, 10.5206, 10.4306
```
I don't think it's possible to come up with an example of where a maplinks based design would have better performance, but it's something for reviewers to consider.
# Discussion
## Why maplinks in the first place?
I spoke with the author of mapLinks (sdaftuar) a while back, and my recollection from our conversation was that it was implemented because he did not know how to resolve the circular dependency at the time, and there was no other reason for making it a separate map.
## Is iterator_to weird?
iterator_to is expressly for this purpose, see https://www.boost.org/doc/libs/1_51_0/libs/multi_index/doc/tutorial/indices.html#iterator_to
> iterator_to provides a way to retrieve an iterator to an element from a pointer to the element, thus making iterators and pointers interchangeable for the purposes of element pointing (not so for traversal) in many situations. This notwithstanding, it is not the aim of iterator_to to promote the usage of pointers as substitutes for real iterators: the latter are specifically designed for handling the elements of a container, and not only benefit from the iterator orientation of container interfaces, but are also capable of exposing many more programming bugs than raw pointers, both at compile and run time. iterator_to is thus meant to be used in scenarios where access via iterators is not suitable or desireable:
>
> - Interoperability with preexisting APIs based on pointers or references.
> - Publication of pointer-based interfaces (for instance, when designing a C-compatible library).
> - The exposure of pointers in place of iterators can act as a type erasure barrier effectively decoupling the user of the code from the implementation detail of which particular container is being used. Similar techniques, like the famous Pimpl idiom, are used in large projects to reduce dependencies and build times.
> - Self-referencing contexts where an element acts upon its owner container and no iterator to itself is available.
In other words, iterator_to is the perfect tool for the job by the last reason given. Under the hood it should just be a simple pointer cast and have no major runtime overhead (depending on if the function call is inlined).
Edit by laanwj: removed at sign from the description
ACKs for top commit:
jonatack:
re-ACK 296be8f per `git range-diff ab338a19 3ba1665 296be8f`, sanity check gcc 10.2 debug build is clean.
hebasto:
re-ACK 296be8f58e02b39a58f017c52294aceed22c3ffd, only rebased since my [previous](https://github.com/bitcoin/bitcoin/pull/19478#pullrequestreview-482400727) review (verified with `git range-diff`).
Tree-SHA512: f5c30a4936fcde6ae32a02823c303b3568a747c2681d11f87df88a149f984a6d3b4c81f391859afbeb68864ef7f6a3d8779f74a58e3de701b3d51f78e498682e
a8a64acaf32ac21feeb885671772282b531ef9a2 [BroadcastTransaction] Remove unsafe move operator (Amiti Uttarwar)
125c0381266e0e05a408f8e1818501ab73d29110 [p2p] Remove dead code (Amiti Uttarwar)
fc66d0a65cdc52a3b259effe0c29b5eafb1b5ff5 [p2p] Check for nullptr before dereferencing pointer (Adam Jonas)
cb79b9dbf4cd06e17c8c65b36bf15c3ea2641de4 [mempool] Revert unbroadcast set to tracking just txid (Amiti Uttarwar)
Pull request description:
Addresses some outstanding review comments from #18044
- reverts unbroadcast txids to a set instead of a map (simpler, communicates intent better, takes less space, no efficiency advantages of map)
- adds safety around two touchpoints (check for nullptr before dereferencing pointer, remove an inaccurate std::move operator)
- removes some dead code
Links to comments on wtxid PR: [1](https://github.com/bitcoin/bitcoin/pull/18044#discussion_r460495254) [2](https://github.com/bitcoin/bitcoin/pull/18044#discussion_r460496023) [3](https://github.com/bitcoin/bitcoin/pull/18044#discussion_r463532611)
thanks to jnewbery & adamjonas for flagging these ! !
ACKs for top commit:
sdaftuar:
utACK a8a64acaf32ac21feeb885671772282b531ef9a2
naumenkogs:
utACK a8a64acaf32ac21feeb885671772282b531ef9a2
jnewbery:
utACK a8a64acaf32ac21feeb885671772282b531ef9a2
Tree-SHA512: 7be669cb30cc17fb9e06b50e636ef7887c6a27354697987e4e4d38dba4b8f50e175647587430cd9bc3295bec01ce8b1e6639a50a4249d8fff9b1ca1b9ead3277
fa362064e383163a2585ffbc71ac1ea3bcc92663 rpc: Return total fee in mempool (MarcoFalke)
Pull request description:
This avoids having to loop over the whole mempool to query each entry's fee
ACKs for top commit:
achow101:
ACK fa362064e383163a2585ffbc71ac1ea3bcc92663
glozow:
ACK fa362064e3🧸
jnewbery:
ACK fa362064e383163a2585ffbc71ac1ea3bcc92663
Tree-SHA512: e2fa1664df39c9e187f9229fc35764ccf436f6f75889c5a206d34fff473fc21efbf2bb143f4ca7895c27659218c22884d0ec4195e7a536a5a96973fc9dd82d08
68537275bd91d1dc14a69609ae443f955bfdbd64 build: Enable -Werror=sign-compare (Ben Woosley)
eac6a3080d38cfd4eb7204ecd327df213958e51a refactor: Rework asmap Interpret to avoid ptrdiff_t (Ben Woosley)
df37377e30678ac9b8338ea920e50b7296da6bd5 test: Fix outstanding -Wsign-compare errors (Ben Woosley)
Pull request description:
Disallowing sign-comparison mismatches can help to prevent the introduction of overflow and interpretation bugs.
In this case, ~all~ most existing violations are in the tests, and most simply required annotating the literal as unsigned for comparison.
This was previously prevented by violations in leveldb which were fixed upstream and merged in #17398. You can test that by building this branch against: 22d11187ee3c7abfe9d43c9eb68f102498cc2b9a vs 75fb37ce68289eb7e00e2ccdd2ef7f9271332545
ACKs for top commit:
fjahr:
re-ACK 68537275bd91d1dc14a69609ae443f955bfdbd64
practicalswift:
ACK 68537275bd91d1dc14a69609ae443f955bfdbd64
Tree-SHA512: 14b5daa38c496fb51548feb30fb4dd179e6f76a8d355f52bc8e2a18f2f9340f0bc98dcf36d8b3d6521045d013891c3103749a4eda88ceef00202a6a0cf93f73c
fd6580e405699ccb051fd2a34525e48d3253673d [refactor] txmempool: split epoch logic into class (Anthony Towns)
Pull request description:
Splits the epoch logic introduced in #17925 into a separate class.
Uses clang's thread safety annotations and encapsulates the data more strongly to reduce chances of bugs from API misuse.
ACKs for top commit:
jonatack:
ACK fd6580e405699ccb051fd2a34525e48d3253673d using clang thread safety annotations looks like a very good idea, and the encapsulation this change adds should improve robustness (and possible unit test-ability) of the code. Verified that changing some of the locking duly provoked build-time warnings with Clang 9 on Debian and that small changes in the new `Epoch` class were covered by failing functional test assertions in `mempool_updatefromblock.py`, `mempool_resurrect.py`, and `mempool_reorg.py`
hebasto:
re-ACK fd6580e405699ccb051fd2a34525e48d3253673d, since my [previous](https://github.com/bitcoin/bitcoin/pull/18017#pullrequestreview-569619362) review:
Tree-SHA512: 7004623faa02b56639aa05ab7a078320a6d8d54ec62d8022876221e33f350f47df51ddff056c0de5be798f8eb39b5c03c2d3f035698555d70abc218e950f2f8c
1e62350ca20898189904a88dfef9ea11ddcd8626 refactor: Improve use of explicit keyword (Fabian Jahr)
c502a6dbfb854ca827a5a3925394f9e09d29b898 lint: Use c++17 std in cppcheck linter (Fabian Jahr)
Pull request description:
I found the `extended-lint-cppcheck` linter still uses `std=c++11` when reviewing #20471. The only difference in the output after this change is one line is missing:
```
src/script/descriptor.cpp:159:5: warning: Struct 'PubkeyProvider' has a constructor with 1 argument that is not explicit. [noExplicitConstructor]
```
After some digging, I am still not sure why this one is ignored with c++17 when 40 other`noExplicitConstructor` warnings were still appearing.
In the second commit, I fix these warnings, adding `explicit` where appropriate and adding fixes to ignore otherwise.
ACKs for top commit:
practicalswift:
cr ACK 1e62350ca20898189904a88dfef9ea11ddcd8626: patch looks correct!
MarcoFalke:
review ACK 1e62350ca20898189904a88dfef9ea11ddcd8626
Tree-SHA512: dff7b324429a57160e217cf38d9ddbb6e70c6cb3d3e3e0bd4013d88e07afc2292c3df94d0acf7122e9d486322821682ecf15c8f2724a78667764c05d47f89a12
## Issue being fixed or feature implemented
This is an implementation of DIP0027 "Credit Asset Locks".
It's a mechanism to fluidly exchange between Dash and credits.
## What was done?
This pull request includes:
- Asset Lock transaction
- Asset Unlock transaction (withdrawal)
- Credit Pool in coinbase
- Unit tests for Asset Lock/Unlock tx
- New functional test `feature_asset_locks.py`
RPC: currently locked amount (credit pool) is available through rpc call
`getblock`.
## How Has This Been Tested?
There added new unit tests for basic checks of transaction validity
(asset lock/unlock).
Also added new functional test "feature_asset_locks.py" that cover
typical cases, but not all corner cases yet.
## Breaking Changes
This feature should be activated as hard-fork because:
- It adds 2 new special transaction and one of them [asset unlock tx]
requires update consensus rulels
- It adds new data in coinbase tx (credit pool)
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
**To release DIP 0027**
- [x] I have assigned this pull request to a milestone
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
## Issue being fixed or feature implemented
Many usages of `CBLS{Signature,PrivateKey,PublicKey}` assume using
global variable, even if can be specified explicitly.
Some of these usages have been deglobalized in this PR.
Some prior improvements and fixes are here:
[#5403](https://github.com/dashpay/dash/pull/5403)
## What was done?
- Refactored the uses of global variable of `bls_legacy_scheme` from
`SetHex`, `SetByteVector`, some rpc calls.
- Removed flag `checkMalleable` to simplify code because it's always
`true`.
- Removed dependency from `txmempool.h` on `bls.h` to speed up
compilation.
## How Has This Been Tested?
Run unit/functional tests.
## Breaking Changes
No breaking changes assumed. But in theory behaviour of some RPC can be
more explicit and predictable.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
## Issue being fixed or feature implemented
same as #5392, alternative solution
~based on #5402 atm, will rebase later~
## What was done?
pls see individual commits
## How Has This Been Tested?
reorg mainnet around forkpoint with a patched client (to allow low
difficulty), run tests
## Breaking Changes
Another evodb migration is required. Going back to an older version or
migrating after the fork requires reindexing.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
e09c701e0110350f78366fb837308c086b6503c0 scripted-diff: Bump copyright of files changed in 2020 (MarcoFalke)
6cbe6209646db8914b87bf6edbc18c6031a16f1e scripted-diff: Replace CCriticalSection with RecursiveMutex (MarcoFalke)
Pull request description:
`RecursiveMutex` better clarifies that the mutex is recursive, see also the standard library naming: https://en.cppreference.com/w/cpp/thread/recursive_mutex
For that reason, and to avoid different people asking me the same question repeatedly (e.g. https://github.com/bitcoin/bitcoin/pull/15932#pullrequestreview-339175124 ), remove the outdated alias `CCriticalSection` with a scripted-diff
eb7b78165966f2c79da71b993c4c4d793e37297f modify p2p_feefilter test to catch rounding error (Gregory Sanders)
6a51f7951716d6d6fc0f9b56028f3a0dd02b61c8 Disallow implicit conversion for CFeeRate constructor (Gregory Sanders)
8e59af55aaf1b196575084bce2448af02d97d745 feefilter: Compute the absolute fee rather than stored rate to match mempool acceptance logic (Gregory Sanders)
Pull request description:
This means we will use the rounding-down behavior in `GetFee` to match both mempool acceptance and wallet logic, with minimal changes.
Fixes https://github.com/bitcoin/bitcoin/issues/16499
Replacement PR for https://github.com/bitcoin/bitcoin/pull/16500
ACKs for top commit:
ajtowns:
ACK eb7b78165966f2c79da71b993c4c4d793e37297f code review only
naumenkogs:
utACK eb7b78165966f2c79da71b993c4c4d793e37297f
achow101:
re ACK eb7b78165966f2c79da71b993c4c4d793e37297f
promag:
ACK eb7b78165966f2c79da71b993c4c4d793e37297f.
Tree-SHA512: 484a11c8f0e825f0c983b1f7e71cf6252b1bba6858194abfe4c088da3bae8a418ec539ef6c4181bf30940e277a95c08d493595d59dfcc6ddf77c65b05563dd7e
9e1cb1adf1800efe429e348650931f2669b0d2c0 [trivial/doc] Fix comment type (Amiti Uttarwar)
8f30260a67166a6ab7c0f33f7ec1990d3c31761e [doc] Update unbroadcast description in RPC results (Amiti Uttarwar)
750456d6f29c63d57af05bfbdd6035bb9c965de2 [trivial] Remove misleading 'const' (Amiti Uttarwar)
fa32e676e5833a5c5fc735ef00c0a80f5fab7a2c [test] Manage node connections better in mempool persist test (Amiti Uttarwar)
1f94bb0c744a103b633c1051e8fbc01e612097dc [doc] Provide rationale for randomization in scheduling. (Amiti Uttarwar)
9c8a55d9cb0ec73f10b196e79b637aa601c0a6b7 [mempool] Don't throw expected error message when upgrading (Amiti Uttarwar)
ba5498318233ab81decbc585e9619d8ffe2df1b0 [test] Test that wallet transactions aren't rebroadcast before 12 hours (Amiti Uttarwar)
00d44a534b4e5ae249b8011360c6b0f7dc731581 [test] P2P connection behavior should meet expectations (Amiti Uttarwar)
bd093ca15de762fdaf0937a0877d17b0c2bce16e [test] updates to unbroadcast test (Amiti Uttarwar)
dab298d9ab5a5a41685f437db9081fa7b395fa73 [docs] add release notes (Amiti Uttarwar)
Pull request description:
This PR is a follow up to #18038 which introduced the idea of an unbroadcast set & focuses mostly on documentation updates and test fixes. One small functionality update to not throw an expected error in `LoadMempool` when you upgrade software versions.
#18895 is another follow up to that addresses other functionality updates.
Background context:
The unbroadcast set is a mechanism for the mempool to track locally submitted transactions (via wallet or RPC). The node does a best-effort of delivering the transactions to the network via retries every 10-15 minutes until either a `GETDATA` is received or the transaction is removed from the mempool.
ACKs for top commit:
MarcoFalke:
ACK 9e1cb1adf1 👁
gzhao408:
ACK [`9e1cb1a`](9e1cb1adf1)
Tree-SHA512: 0cd51c4ca368b9dce92d50d73ec6e9df278a259e609eef2858f24cb8595ad07acc3db781d9eb0c351715f18fca5a2b4526838981fdb34a522427e9dc868bdaa6
651f1d816f054cb9c637f8a99c9360bba381ef58 [test] wait for inital broadcast before comparing mempool entries (gzhao408)
9d3f7eb9860254eb787ebe2734fd6a26bcf365c1 [mempool] sanity check that all unbroadcast txns are in mempool (gzhao408)
a7ebe48b94c5a9195c8eabd193204c499cb4bfdb [rpc] add unbroadcast info to mempool entries and getmempoolinfo (gzhao408)
d16006960443c2efe37c896e46edae9dca86c57d [wallet] remove nLastResend logic (gzhao408)
Pull request description:
Followup to #18038 by amitiuttarwar which introduces the unbroadcast set: "a mechanism for the mempool to track locally submitted transactions" and decreases the frequency of rebroadcast from 10-15 minutes to 12-36 hours.
This PR addresses some of the outstanding TODOs building on top of it:
- remove `nLastResend` logic, which is used to ensure rebroadcast doesn't happen again if no new block has been mined (makes sense in 10-15 min period, but not necessary for 12-36 hour period). (#18038 [comment](https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416826914))
- expose unbroadcast info via RPCs, for more informative queries and testing (#18038 [comment](https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416837980))
- add sanity check to verify unbroadcast transactions still exist in mempool before rebroadcasting (#18038 [comment](https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416861609))
ACKs for top commit:
naumenkogs:
Code review ACK 651f1d816f054cb9c637f8a99c9360bba381ef58
amitiuttarwar:
ACK 651f1d816f054cb9c637f8a99c9360bba381ef58 🎉
MarcoFalke:
Review ACK 651f1d816f054cb9c637f8a99c9360bba381ef58
Tree-SHA512: d5327e95ef39d44152b48df5c610502ae11c168f43dbbfb2885340c93d1ba9426eb3a5794573f5fc843502109cb3ffb63efa3f2db4f8f112efcde8f76d9a8845
020f0519ec66d9626255b938e1c6c3f7f9aa4017 refactor: CTxMemPool::IsUnbroadcastTx() requires CTxMemPool::cs lock (Hennadii Stepanov)
7c4bd0387a01a0c3e2938d530dba3c882e4d8f2b refactor: CTxMemPool::GetTotalTxSize() requires CTxMemPool::cs lock (Hennadii Stepanov)
fa5fcb032b6ed04c49ee465235288b8059fa805e refactor: CTxMemPool::ClearPrioritisation() requires CTxMemPool::cs lock (Hennadii Stepanov)
7140b31b90cbd84d75eedb3e395d0d55f83b5b95 refactor: CTxMemPool::ApplyDelta() requires CTxMemPool::cs lock (Hennadii Stepanov)
66e47e5e506043fbb9b4e487b44bf992985709c9 refactor: CTxMemPool::UpdateChild() requires CTxMemPool::cs lock (Hennadii Stepanov)
939807768acd508932f2efabee660d56324a73df refactor: CTxMemPool::UpdateParent() requires CTxMemPool::cs lock (Hennadii Stepanov)
Pull request description:
This is another step to transit `CTxMemPool::cs` from `RecursiveMutex` to `Mutex`.
Split out from #19306.
Only trivial thread safety annotations and lock assertions added. No new locks. No behavior change.
Refactoring `const uint256` to `const uint256&` was [requested](https://github.com/bitcoin/bitcoin/pull/19647#discussion_r468471022) by **promag**.
Please note that now, since #19668 has been merged, it is safe to apply `AssertLockHeld()` macros as they do not swallow compile time Thread Safety Analysis warnings.
ACKs for top commit:
promag:
Core review ACK 020f0519ec66d9626255b938e1c6c3f7f9aa4017.
jnewbery:
Code review ACK 020f0519ec66d9626255b938e1c6c3f7f9aa4017
vasild:
ACK 020f0519e
Tree-SHA512: a31e389142d5a19b25fef0aaf1072a337278564528b5cc9209df88ae548a31440e1b8dd9bae0169fd7aa59ea06e22fe5e0413955386512b83ef1f3e7d941e890
e57980b4738c10344baf136de3e050a3cb958ca5 [mempool] Remove NotifyEntryAdded and NotifyEntryRemoved callbacks (John Newbery)
2dd561f36124972d2364f941de9c3417c65f05b6 [validation] Remove pool member from ConnectTrace (John Newbery)
969b65f3f527631ede1a31c7855151e5c5d91f8f [validation] Remove NotifyEntryRemoved callback from ConnectTrace (John Newbery)
5613f9842b4000fed088b8cf7b99674c328d15e1 [validation] Remove conflictedTxs from PerBlockConnectTrace (John Newbery)
cdb893443cc16edf974f099b8485e04b3db1b1d7 [validation interface] Remove vtxConflicted from BlockConnected (John Newbery)
1168394d759b13af68acec6d5bfa04aaa24561f8 [wallet] Notify conflicted transactions in TransactionRemovedFromMempool (John Newbery)
Pull request description:
These boost signals were added in #9371, before we had a `TransactionRemovedFromMempool` method in the validation interface. The `NotifyEntryAdded` callback was used by validation to build a vector of conflicted transactions when connecting a block, which the wallet was notified of in the `BlockConnected` CValidationInterface callback.
Now that we have a `TransactionRemovedFromMempool` callback, we can fire that signal directly from the mempool for conflicted transactions.
Note that #9371 was implemented to ensure `-walletnotify` events were fired for these conflicted transaction. We inadvertently stopped sending these notifications in #16624 (Sep 2019 commit 7e89994). We should probably fix that, but in a different PR.
ACKs for top commit:
jonatack:
Re-ACK e57980b
ryanofsky:
Code review ACK e57980b4738c10344baf136de3e050a3cb958ca5, no code changes since previous review, but helpful new code comments have been added and the PR description is now more clear about where the old code came from
Tree-SHA512: 3bdbaf1ef2731e788462d4756e69c42a1efdcf168691ce1bbfdaa4b7b55ac3c5b1fd4ab7b90bcdec653703600501b4224d252cfc086aef28f9ce0da3b0563a69
faec689bed7a5b66e2a7675853d10205b933cec8 txmempool: Make entry time type-safe (std::chrono) (MarcoFalke)
faaa1f01daba94b021ca77515266a16d27f0364e util: Add count_seconds time helper (MarcoFalke)
1111170f2f0141084b5b4ed565b2f07eba48599a test: mempool entry time is persisted (MarcoFalke)
Pull request description:
This changes the type of the entry time of txs into the mempool from `int64_t` to `std::chrono::seconds`.
The benefits:
* Documents the type for developers
* Type violations result in compile errors
* After compilation, the two are equivalent (at no run time cost)
ACKs for top commit:
ajtowns:
utACK faec689bed7a5b66e2a7675853d10205b933cec8
laanwj:
ACK faec689bed7a5b66e2a7675853d10205b933cec8
Tree-SHA512: d958e058755d1a1d54cef536a8b30a11cc502b7df0d6ecf84a0ab1d38bc8105a67668a99cd5087a444f6de2421238111c5fca133cdf8e2e2273cb12cb6957845
d314e8a818d4c162b1c7201533e6b600dcab2d91 refactor: Replace all uses of boost::optional with our own Optional type (Wladimir J. van der Laan)
Pull request description:
Replace all uses of boost::optional with our own Optional type. Luckily, there aren't so many.
After this:
- `boost::optional` is no longer used directly (only through `Optional` which is an alias for it)
- `boost/optional.hpp` is only included in one place
ACKs for top commit:
MarcoFalke:
ACK d314e8a818d4c162b1c7201533e6b600dcab2d91
practicalswift:
ACK d314e8a818d4c162b1c7201533e6b600dcab2d91 -- diff looks correct + satisfying to see incremental progress towards the goal of a Boost free future :)
jtimon:
ACK d314e8a818d4c162b1c7201533e6b600dcab2d91
fanquake:
ACK d314e8a818d4c162b1c7201533e6b600dcab2d91
Tree-SHA512: b43e0017af81b07b5851377cd09624f114510ac5b9018d037664b58ad0fc8e893e30946b61f8f5e21e39125925bf9998a81f2226b468aab2df653ee57ed3213d
effe81f750 Move g_is_mempool_loaded into CTxMemPool::m_is_loaded (Ben Woosley)
bb8ae2c419 rpc: Expose g_is_mempool_loaded via getmempoolinfo and /rest/mempool/info.json (Ben Woosley)
Pull request description:
And use it to fix a race condition in mempool_persist.py:
https://travis-ci.org/Empact/bitcoin/jobs/487577243
Since e.g. getrawmempool returns errors based on this status, this
enables users to test it for readiness.
Fixes#12863
ACKs for commit effe81:
MarcoFalke:
utACK effe81f750
jnewbery:
utACK effe81f7503d2ca3c88cfdea687f9f997f353e0d
Tree-SHA512: 74328b0c17a97efb8a000d4ee49b9a673c2b6dde7ea30c43a6a2eff961a233351c9471f9a42344412135786c02bdf2ee1b2526651bb8fed68bd94d2120c4ef86
0000ff0aa763e3be524ac4537a41048a26529fb2 txmempool: Remove unused default value MemPoolRemovalReason::UNKNOWN (MarcoFalke)
Pull request description:
The `remove*` methods set the removal reason to `UNKNOWN` by default. This is nowhere used; Except in tests, where the value doesn't matter. Fix that by removing the confusing default.
ACKs for top commit:
practicalswift:
utACK 0000ff0aa763e3be524ac4537a41048a26529fb2
promag:
ACK 0000ff0aa763e3be524ac4537a41048a26529fb2.
jonasschnelli:
utACK 0000ff0aa763e3be524ac4537a41048a26529fb2
Tree-SHA512: ffc8b35dd3291a81225171577c743c8bb2645638cab02960b6361174cb68afd739aaab7ab8661d65de5750d37daf16bb7eee9338958d8609093a8d46c2ada1ab
084e17cebd424b8e8ced674bc810eef4e6ee5d3b Remove unused includes (practicalswift)
Pull request description:
As requested by MarcoFalke in https://github.com/bitcoin/bitcoin/pull/16273#issuecomment-521332089:
This PR removes unused includes.
Please note that in contrast to #16273 I'm limiting the scope to the trivial cases of pure removals (i.e. no includes added) to make reviewing easier.
I'm seeking "Concept ACK":s for this obviously non-urgent minor cleanup.
Rationale:
* Avoids unnecessary re-compiles in case of header changes.
* Makes reasoning about code dependencies easier.
* Reduces compile-time memory usage.
* Reduces compilation time.
* Warm fuzzy feeling of being lean :-)
ACKs for top commit:
ryanofsky:
Code review ACK 084e17cebd424b8e8ced674bc810eef4e6ee5d3b. PR only removes include lines and it still compiles. In the worst case someone might have to explicitly add an include later for something now included implicitly. But maybe some effort was taken to avoid this, and it wouldn't be a tragedy anyway.
Tree-SHA512: 89de56edc6ceea4696e9579bccff10c80080821685b9fb4e8c5ef593b6e43cf662f358788701bb09f84867693f66b2e4db035b92b522a0a775f50b7ecffd6a6d
851380ce17 remove deprecated mentions of signrawtransaction from fundraw help (Gregory Sanders)
Pull request description:
RPC call has been removed as of 0.17.99.
Tree-SHA512: a6a12a0e4572acd9b532c1719be85ed6f29d1c1a28f9ce691398528b8dde4fb4a3222b8f68632fcb1a8eddfe2d31e96d5efd5bc51c041af8e7cb99b61ca3a167
fa5e373365 validation: Add cs_main locking annotations (MarcoFalke)
fa5c346c5a doc: Add comment to cs_main and mempool::cs (MarcoFalke)
fafe941bdd test: Add missing validation locks (MarcoFalke)
fac4558462 sync: Add RecursiveMutex type alias (MarcoFalke)
Pull request description:
Both the chain state and the transaction pool are validation specific, but access to them is protected by two locks. The two locks have the following semantics:
* Writing to the chain state or adding transactions to the transaction pool -> Take both `cs_main` and `mempool::cs`
* Reading either or removing transactions from the the transaction pool -> Take only the appropriate lock
Tree-SHA512: 6f6e612ffc391904c6434a79a4f3f8de1b928bf0a3e3434b73561037b395e2b40a70a5a4bd8472dd230e9eacc8e5d5374c904a3c509910cf3971dd7ff59a626c
fa69ac7614 doxygen: Fix member comments (MarcoFalke)
Pull request description:
Trailing comments must be indicted with the caret `//!<`.
Not all places do this right now, see for example https://dev.visucore.com/bitcoin/doxygen/txmempool_8h.html#a2bc6653552b5871101b6cbefdbaf251f, but they can be fixed with an almost-scripted-diff:
```
sed -i --regexp-extended -e 's/((,|;) *\/\/!) /\1< /g' $(git grep --extended-regexp -l '(,|;)\s*//!\s')
```
(Same as [doxygen] Fix member comments #7793)
Tree-SHA512: 451077008353ccc6fcc795f34094b2d022feb7a171b562a07ba4de0dcb0aebc137e12b03970764bd81e2da386751d042903db4c4831900f43c0cfde804c81b2b
faa1a749428a195af784633eb78e1df5d6a0e875 tx pool: Use class methods to hide raw map iterator impl details (MarcoFalke)
Pull request description:
ATMP et al would often use map iterator implementation details such as
`end()` or `find()`, which is acceptable in current code.
However, this not only makes it impossible to turn the maps into
private members in the future but also makes it harder to replace the
maps with different data structures.
This is required for and split off of #13804
Tree-SHA512: 4f9017fd1d98d9df49d25bba92655a4a97755eea161fd1cbb565ceb81bbc2b4924129d214f8a29563a77e3d8eef85a67c81245ecdc9a9e5292d419922a93cb88
* refactor/bls: misc refactoring and spelling/grammar fixes in bls code
* refactor/evo: misc refactoring and spelling/grammar fixes in evo code
* refactor: some include changes
* refactor: remove redundant `public`
* fix linter
Signed-off-by: pasta <pasta@dashboost.org>
* Sort includes
* Move `class CTxDSIn;`
* Drop unused functions in CBLSWorker
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
