| Age | Commit message (Collapse) | Author |
|---|
|
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
|
|
Several other parameters are now redundant since they can be safely
obtained from the chainstate given that ::cs_main is locked. These are
now removed.
|
|
|
|
|
|
-BEGIN VERIFY SCRIPT-
find_regex='\bAcceptToMemoryPool\(' \
&& git grep -l -E "$find_regex" -- src \
| grep -v '^src/validation\.\(cpp\|h\)$' \
| xargs sed -i -E 's@'"$find_regex"'@\0::ChainstateActive(), @g'
-END VERIFY SCRIPT-
|
|
|
|
-BEGIN VERIFY SCRIPT-
find_regex='\bCheckFinalTx\(' \
&& git grep -l -E "$find_regex" -- src \
| grep -v '^src/validation\.\(cpp\|h\)$' \
| xargs sed -i -E 's@'"$find_regex"'@\0::ChainActive().Tip(), @g'
-END VERIFY SCRIPT-
|
|
|
|
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 https://github.com/bitcoin/bitcoin/pull/20944/commits/fa362064e383163a2585ffbc71ac1ea3bcc92663 🧸
jnewbery:
ACK fa362064e383163a2585ffbc71ac1ea3bcc92663
Tree-SHA512: e2fa1664df39c9e187f9229fc35764ccf436f6f75889c5a206d34fff473fc21efbf2bb143f4ca7895c27659218c22884d0ec4195e7a536a5a96973fc9dd82d08
|
|
[META] This commit should be followed up by removing the comments and
assertions meant only to show that the change is correct.
GetSpendHeight only acts on BlockManager.
|
|
Also, add missing lock annotations
|
|
Currently, CTxMemPool::check locks CTxMemPool's own cs member, then
calls GetSpendHeight which locks cs_main. This can potentially cause an
undesirable lock invesion since CTxMemPool's cs is supposed to be locked
after cs_main.
This does not cause us any problems right now because all callers of
CTxMemPool already lock cs_main before calling CTxMemPool::check, which
means that the LOCK(cs_main) in GetSpendHeight becomes benign.
However, it is currently possible for new code to be added which calls
CTxMemPool::check without locking cs_main (which would be dangerous).
Therefore we should make it explicit that cs_main needs to be held
before calling CTxMemPool::check.
NOTE: After all review-only assertions are removed in "#20158 |
tree-wide: De-globalize ChainstateManager", and assuming that we
keep the changes in "validation: Pass in spendheight to
CTxMemPool::check", we can re-evaluate to see if this annotation
is still necessary.
|
|
281fd1a4a032cded7f9ea9857e3e99fc793c714b Replace KeyIDHasher with SaltedSipHasher (Andrew Chow)
210b693db66e7c5b618014b5a287aee15af00045 Add generic SaltedSipHasher (Andrew Chow)
95e61c1cf2a91d041c8025306ba36f0ea2806894 Move Hashers to util/hasher.{cpp/h} (Andrew Chow)
Pull request description:
There are existing `SaltedOutPointHasher` and `SaltedTxidHasher` classes used for `std::unordered_map` and `std::unordered_set` that could be useful in other places in the codebase. So we these to their own `saltedhash.{cpp/h}` file. An existing `KeyIDHasher` is moved there too. Additionally, `ScriptIDHasher`, `SaltedPubkeyHasher`, and `SaltedScriptHasher` are added so that they can be used in future work.
`KeyIDHasher` and `ScriptIDHasher` are not salted so that equality comparisons of maps and sets keyed by `CKeyID` and `CScriptID` will actually work.
Split from #19602 (and a few other PRs/branches I have).
ACKs for top commit:
laanwj:
Code review ACK 281fd1a4a032cded7f9ea9857e3e99fc793c714b
jonatack:
ACK 281fd1a4a032cded7f9ea9857e3e99fc793c714b, code review, debug build and ran bitcoind after rebasing to master @ dff0f6f753ea
fjahr:
utACK 281fd1a4a032cded7f9ea9857e3e99fc793c714b
Tree-SHA512: bb03b231ccf3c9ecefc997b8da9c3770af4819f9be5b0a72997a103864e84046a2ac39b8eadf0dc9247bdccd53f86f433642e3a098882e6748341a9e7736271b
|
|
Move the hashers that we use for hash tables to a common place.
Moved hashers:
- SaltedTxidHasher
- SaltedOutpointHasher
- FilterHeaderHasher
- SignatureCacheHasher
- BlockHasher
|
|
Shorten the CTxMemPool initializer list using default initialization
for members that dont depend on the constuctor parameters.
|
|
Since m_check_ratio is only set once and since the CTxMemPool object is
no longer a global variable, m_check_ratio can be passed into the
constructor of CTxMemPool. Since it is only read from after
initialization, m_check_ratio can also be made a const and hence no
longer needs to be guarded by the cs mutex.
|
|
Use a ratio instead of a frequency that requires a double to int cast
for determining how often a mempool sanity check should run.
|
|
Using the zmq notifications to avoid excessive mempool polling can be difficult
given the current notifications available. It announces all transactions
being added to mempool or included in blocks, but announces no evictions
and gives no indication if the transaction is in the mempool or a block.
Block notifications for zmq are also substandard, in that it only announces
block tips, while all block transactions are still announced.
This commit adds a unified stream which can be used to closely track mempool:
1) getrawmempool to fill out mempool knowledge
2) if txhash is announced, add or remove from set
based on add/remove flag
3) if blockhash is announced, get block txn list,
remove from those transactions local view of mempool
4) if we drop a sequence number, go to (1)
The mempool sequence number starts at the value 1, and
increments each time a transaction enters the mempool,
or is evicted from the mempool for any reason, including
block inclusion. The mempool sequence number is published
via ZMQ for any transaction-related notification.
These features allow for ZMQ/RPC consumer to track mempool
state in a more exacting way, without unnecesarily polling
getrawmempool. See interface_zmq.py::test_mempool_sync for
example usage.
|
|
CTxMemPool::GetMemPoolParents
|
|
|
|
No change in behavior, the lock is already held at call sites.
Also `const uint256` refactored to `const uint256&`.
|
|
No change in behavior, the lock is already held at call sites.
Also `const uint256` refactored to `const uint256&`.
|
|
No change in behavior, the lock is already held at call sites.
|
|
No change in behavior, the lock is already held at call sites.
|
|
|
|
|
|
This fix is a based on the fix by Antoine Riard <ariard@student.42.fr> in
https://github.com/bitcoin/bitcoin/pull/18600.
Unlike that PR, which implements some new behavior, this just restores previous
wallet notification and status behavior for transactions removed from the
mempool because they conflict with transactions in a block. The behavior was
accidentally changed in two `CWallet::BlockConnected` updates:
a31be09bfd77eed497a8e251d31358e16e2f2eb1 and
7e89994133725125dddbfa8d45484e3b9ed51c6e from
https://github.com/bitcoin/bitcoin/pull/16624, causing issue
https://github.com/bitcoin/bitcoin/issues/18325.
The change here could be improved and replaced with a more comprehensive
cleanup, so it includes a detailed comment explaining future considerations.
Fixes #18325
Co-authored-by: Antoine Riard <ariard@student.42.fr>
|
|
wallet privacy
50fc4df6c4e8a84bdda13ade7bed7a2131796f00 [mempool] Persist unbroadcast set to mempool.dat (Amiti Uttarwar)
297a1785360c4db662a7f3d3ade7b6b503258d39 [test] Integration tests for unbroadcast functionality (Amiti Uttarwar)
6851502472d3625416f0e7796e9f2a0379d14d49 [refactor/test] Extract P2PTxInvStore into test framework (Amiti Uttarwar)
dc1da48dc5e5526215561311c184a8cbc345ecdc [wallet] Update the rebroadcast frequency to be ~1/day. (Amiti Uttarwar)
e25e42f20a3aa39651fbc1f9fa3df1a49f1f5868 [p2p] Reattempt initial send of unbroadcast transactions (Amiti Uttarwar)
7e93eecce3bc5a1b7bb0284e06f9e2e69454f5ba [util] Add method that returns random time in milliseconds (Amiti Uttarwar)
89eeb4a3335f8e871cc3f5286af4546dff66172a [mempool] Track "unbroadcast" transactions (Amiti Uttarwar)
Pull request description:
This PR introduces mempool tracking of unbroadcast transactions and periodic reattempts at initial broadcast. This is a part of the rebroadcast project, and a standalone privacy win.
The current rebroadcast logic is terrible for privacy because 1. only the source wallet rebroadcasts transactions and 2. it does so quite frequently. In the current system, if a user submits a transaction that does not immediately get broadcast to the network (eg. they are offline), this "rebroadcast" behavior is the safety net that can actually serve as the initial broadcast. So, keeping the attempts frequent is important for initial delivery within a reasonable timespan.
This PR aims to improve # 2 by reducing the wallet rebroadcast frequency to ~1/day from ~1/15 min. It achieves this by separating the notion of initial broadcast from rebroadcasts. With these changes, the mempool tracks locally submitted transactions & periodically reattempts initial broadcast. Transactions submitted via the wallet or RPC are added to an "unbroadcast" set & are removed when a peer sends a `getdata` request, or the transaction is removed from the mempool. Every 10-15 minutes, the node reattempts an initial broadcast. This enables reducing the wallet rebroadcast frequency while ensuring the transactions will be propagated to the network.
For privacy improvements around # 1, please see #16698.
Thank you to gmaxwell for the idea of how to break out this subset of functionality (https://github.com/bitcoin/bitcoin/pull/16698#issuecomment-571399346)
ACKs for top commit:
fjahr:
Code review ACK 50fc4df6c4e8a84bdda13ade7bed7a2131796f00
MarcoFalke:
ACK 50fc4df6c4e8a84bdda13ade7bed7a2131796f00, I think this is ready for merge now 👻
amitiuttarwar:
The current tip `50fc4df` currently has 6 ACKs on it, so I've opened #18807 to address the last bits.
jnewbery:
utACK 50fc4df6c4e8a84bdda13ade7bed7a2131796f00.
ariard:
Code Review ACK 50fc4df (minor points no need to invalid other ACKs)
robot-visions:
ACK 50fc4df6c4e8a84bdda13ade7bed7a2131796f00
sipa:
utACK 50fc4df6c4e8a84bdda13ade7bed7a2131796f00
naumenkogs:
utACK 50fc4df
Tree-SHA512: 2dd935d645d5e209f8abf87bfaa3ef0e4492705ce7e89ea64279cb27ffd37f4727fa94ad62d41be331177332f8edbebf3c7f4972f8cda10dd951b80a28ab3c0f
|
|
- Mempool tracks locally submitted transactions (wallet or rpc)
- Transactions are removed from set when the node receives a GETDATA request
from a peer, or if the transaction is removed from the mempool.
|
|
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
|
|
NotifyEntryAdded never had any subscribers so can be removed.
Since ConnectTrace no longer subscribes to NotifyEntryRemoved, there are
now no subscribers.
The CValidationInterface TransactionAddedToMempool and
TransactionRemovedFromMempool methods can now provide this
functionality. There's no need for a special notifications framework for
the mempool.
|
|
The only CValidationInterface client that cares about transactions that
are removed from the mempool because of CONFLICT is the wallet.
Start using the TransactionRemovedFromMempool method to notify about
conflicted transactions instead of using the vtxConflicted vector in
BlockConnected.
|
|
|
|
|
|
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
|
|
This commit fires TransactionRemovedFromMempool callbacks from the
mempool and cleans up a bunch of code.
|
|
Optional type
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
|
|
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
|
|
Split CValidationState into TxValidationState and BlockValidationState
to store validation results for transactions and blocks respectively.
|
|
acceptance logic
|
|
|
|
|
|
|
|
|
|
|
|
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
|
|
This moves the following policy settings functions and globals to a new
src/policy/settings unit in lib_server:
- `incrementalRelayFee`
- `dustRelayFee`
- `nBytesPerSigOp`
- `fIsBareMultisigStd`
These settings are only required by the node and should not be accessed
by other libraries.
|
|
So the loaded state is explicitly mempool-specific.
|
|
|
|
-BEGIN VERIFY SCRIPT-
mkdir -p src/util
git mv src/util.h src/util/system.h
git mv src/util.cpp src/util/system.cpp
git mv src/utilmemory.h src/util/memory.h
git mv src/utilmoneystr.h src/util/moneystr.h
git mv src/utilmoneystr.cpp src/util/moneystr.cpp
git mv src/utilstrencodings.h src/util/strencodings.h
git mv src/utilstrencodings.cpp src/util/strencodings.cpp
git mv src/utiltime.h src/util/time.h
git mv src/utiltime.cpp src/util/time.cpp
sed -i 's/<util\.h>/<util\/system\.h>/g' $(git ls-files 'src/*.h' 'src/*.cpp')
sed -i 's/<utilmemory\.h>/<util\/memory\.h>/g' $(git ls-files 'src/*.h' 'src/*.cpp')
sed -i 's/<utilmoneystr\.h>/<util\/moneystr\.h>/g' $(git ls-files 'src/*.h' 'src/*.cpp')
sed -i 's/<utilstrencodings\.h>/<util\/strencodings\.h>/g' $(git ls-files 'src/*.h' 'src/*.cpp')
sed -i 's/<utiltime\.h>/<util\/time\.h>/g' $(git ls-files 'src/*.h' 'src/*.cpp')
sed -i 's/BITCOIN_UTIL_H/BITCOIN_UTIL_SYSTEM_H/g' src/util/system.h
sed -i 's/BITCOIN_UTILMEMORY_H/BITCOIN_UTIL_MEMORY_H/g' src/util/memory.h
sed -i 's/BITCOIN_UTILMONEYSTR_H/BITCOIN_UTIL_MONEYSTR_H/g' src/util/moneystr.h
sed -i 's/BITCOIN_UTILSTRENCODINGS_H/BITCOIN_UTIL_STRENCODINGS_H/g' src/util/strencodings.h
sed -i 's/BITCOIN_UTILTIME_H/BITCOIN_UTIL_TIME_H/g' src/util/time.h
sed -i 's/ util\.\(h\|cpp\)/ util\/system\.\1/g' src/Makefile.am
sed -i 's/utilmemory\.\(h\|cpp\)/util\/memory\.\1/g' src/Makefile.am
sed -i 's/utilmoneystr\.\(h\|cpp\)/util\/moneystr\.\1/g' src/Makefile.am
sed -i 's/utilstrencodings\.\(h\|cpp\)/util\/strencodings\.\1/g' src/Makefile.am
sed -i 's/utiltime\.\(h\|cpp\)/util\/time\.\1/g' src/Makefile.am
sed -i 's/-> util ->/-> util\/system ->/' test/lint/lint-circular-dependencies.sh
sed -i 's/src\/util\.cpp/src\/util\/system\.cpp/g' test/lint/lint-format-strings.py test/lint/lint-locale-dependence.sh
sed -i 's/src\/utilmoneystr\.cpp/src\/util\/moneystr\.cpp/g' test/lint/lint-locale-dependence.sh
sed -i 's/src\/utilstrencodings\.\(h\|cpp\)/src\/util\/strencodings\.\1/g' test/lint/lint-locale-dependence.sh
sed -i 's/src\\utilstrencodings\.cpp/src\\util\\strencodings\.cpp/' build_msvc/libbitcoinconsensus/libbitcoinconsensus.vcxproj
-END VERIFY SCRIPT-
|
