37371268d14ed6d5739af5b65d8bdb38b0e8dda2 Mark `CheckTxInputs` `[[nodiscard]]` (out-param `txfee` only set if call is successful). Avoid UUM in fuzzing harness `coins_view`. (practicalswift)
Pull request description:
Mark `CheckTxInputs` `[[nodiscard]]` (out-param `txfee` only set if call is successful).
Avoid use of uninitialised memory (UUM) in fuzzing harness `coins_view`.
ACKs for top commit:
MarcoFalke:
review ACK 37371268d14ed6d5739af5b65d8bdb38b0e8dda2
Tree-SHA512: edada5b2e80ce9ad3bd57b4c445bedefffa0a2d1cc880957d6848e4b7d9fc1ce036cd17f8b18bc03a36fbf84fc29c166cd6ac3dfbfe03e69d6fdbda13697754d
6c3fcd5591eb9947f35483014ecb0d8ab217b780 test: remove BasicTestingSetup from util_threadnames unit tests (fanquake)
b53d3c1b1fd739c314b0b34f361fcd992092fc29 test: remove BasicTestingSetup from uint256 unit tests (fanquake)
c0497a49281e68b57e2a1e6c48c950b2edc80821 test: remove BasicTestingSetup from torcontrol unit tests (fanquake)
ef8bb0473be62c07f96eb269b927dcec86c1e862 test: remove BasicTestingSetup from sync unit tests (fanquake)
1aee83421fe2128757b48f6317a3e7fed784adb6 test: remove BasicTestingSetup from reverse_lock unit tests (fanquake)
57ba949ef585f8124914c43ea9a53afee201b998 test: remove BasicTestingSetup from policy_fee unit tests (fanquake)
3974c962b61a1e18f8177ffa30791ad9ad2ba6e4 test: remove BasicTestingSetup from merkleblock tests (fanquake)
cd5bc4b4708b28cabcfabbcd7f5ba1155f5b1517 test: remove BasicTestingSetup from hash unit tests (fanquake)
39cec22935302418963cc2e7db4ad2fa9656849d test: remove BasicTestingSetup from compilerbug unit tests (fanquake)
6d3b78c0e2f427d3a7431885cc175464a527a12a test: remove BasicTestingSetup from bswap unit tests (fanquake)
a13dc24831e4a2d8e16a41d8c95cdaa8afdec783 test: remove BasicTestingSetup from bech32 unit tests (fanquake)
f4dcbe4498e55d2ed818b35cd15652fd427b7a7b test: remove BasicTestingSetup from base64 unit tests (fanquake)
fd144f64265a4752fe36391c51bb6b8ccdff838f test: remove BasicTestingSetup from base32 unit tests (fanquake)
4c389ba04b36cc2916d49435e07155144882a637 test: remove BasicTestingSetup from arith_uint256 unit tests (fanquake)
05590651a0b9ebc5f5fdbdcbbc1efe4bf64888d0 test: remove BasicTestingSetup from amount unit tests (fanquake)
883a5c7d021fe29539d417796a5b07e265f1c696 test: remove BasicTestingSetup from allocator unit tests (fanquake)
Pull request description:
* Less setup/overhead for tests that don't need it. Some naive bench-marking would suggest that a full `test_bitcoin` run is a few % faster after this change.
* Tests which don't need the BasicTestingSetup can't accidentally end up depending on it somehow.
* Already the case in at least the scheduler and block_filter tests.
This adds missing includes, but more significant is the removal of `setup_common.h` from tests where it isn't needed. This saves recompiling those tests when changes are made in the header.
ACKs for top commit:
practicalswift:
cr ACK 6c3fcd5591eb9947f35483014ecb0d8ab217b780: patch looks correct
laanwj:
ACK 6c3fcd5591eb9947f35483014ecb0d8ab217b780
Tree-SHA512: 69b891e2b4740402d62b86a4fc98c329a432d125971342a6f97334e166b3537ed3d4cdbb2531fa05c1feae32339c9fcb2dceda9afeeaed4edc70e8caa0962161
7a799c9c2b8652e780d1fd5e1bf7d05b026c1c1a index: refactor-only: Reuse CChain ref (Carl Dong)
db33cde80fff749c6adff9e91fca5f27f4bb6278 index: Add chainstate member to BaseIndex (Carl Dong)
f4a47a1febfa35ab077f2a841fe31a8cd9618250 bench: Use existing chainman in AssembleBlock (Carl Dong)
91226eb91769aad5a63bc671595e1353a2b2247a bench: Use existing NodeContext in DuplicateInputs (Carl Dong)
e6b4aa6eb53dc555ecab2922af35e7a2572faf4f miner: Pass in chainman to RegenerateCommitments (Carl Dong)
9ecade14252ad1972f668d2d2e4ef44fdfcb944a rest: Add GetChainman function and use it (Carl Dong)
fc1c282845f6b8436d1ea4c68eb3511034c29bea rpc/blockchain: Use existing blockman in gettxoutsetinfo (Carl Dong)
Pull request description:
Overall PR: #20158 (tree-wide: De-globalize ChainstateManager)
The first 2 commits are fixups addressing review for the last bundle: #21391
NEW note:
1. I have opened #21766 which keeps track of potential improvements where the flaws already existed before the de-globalization work, please post on that issue about these improvements, thanks!
Note to reviewers:
1. This bundle may _apparently_ introduce usage of `g_chainman` or `::Chain(state|)Active()` globals, but these are resolved later on in the overall PR. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
2. There may be seemingly obvious local references to `ChainstateManager` or other validation objects which are not being used in callers of the current function in question, this is done intentionally to **_keep each commit centered around one function/method_** to ease review and to make the overall change systematic. We don't assume anything about our callers. Rest assured that once we are considering that particular caller in later commits, we will use the obvious local references. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
3. When changing a function/method that has many callers (e.g. `LookupBlockIndex` with 55 callers), it is sometimes easier (and less error-prone) to use a scripted-diff. When doing so, there will be 3 commits in sequence so that every commit compiles like so:
1. Add `new_function`, make `old_function` a wrapper of `new_function`, divert all calls to `old_function` to `new_function` **in the local module only**
2. Scripted-diff to divert all calls to `old_function` to `new_function` **in the rest of the codebase**
3. Remove `old_function`
ACKs for top commit:
jarolrod:
ACK 7a799c9
ariard:
Code Review ACK 7a799c9
fjahr:
re-ACK 7a799c9c2b8652e780d1fd5e1bf7d05b026c1c1a
MarcoFalke:
review ACK 7a799c9c2b8652e780d1fd5e1bf7d05b026c1c1a 🌠
ryanofsky:
Code review ACK 7a799c9c2b8652e780d1fd5e1bf7d05b026c1c1a. Basically no change since last review except fixed rebase conflicts and a new comment about REST Ensure()
jamesob:
conditional ACK 7a799c9c2b8652e780d1fd5e1bf7d05b026c1c1a ([`jamesob/ackr/21767.1.dongcarl.bundle_6_n_prune_g_chai`](https://github.com/jamesob/bitcoin/tree/ackr/21767.1.dongcarl.bundle_6_n_prune_g_chai))
Tree-SHA512: 531c00ddcb318817457db2812d9a9d930bc664e58e6f7f1c746350732b031dd624270bfa6b9f49d8056aeb6321d973f0e38e4ff914acd6768edd8602c017d10e
ffff0d04425a616c14fc4a562e8ef93d286705f8 refactor: Switch serialize to uint8_t (1/n) (MarcoFalke)
Pull request description:
Replace `char` -> `uint8_t` in serialization where a sign doesn't make sense (char might be signed/unsigned).
ACKs for top commit:
practicalswift:
cr ACK ffff0d04425a616c14fc4a562e8ef93d286705f8: patch looks correct and commit hash is ffffresh (was bbbbadass)
kristapsk:
ACK ffff0d04425a616c14fc4a562e8ef93d286705f8
Tree-SHA512: cda682280c21d37cc3a6abd62569732079b31d18df3f157aa28bed80bd6f9f29a7db5c133b1f57b3a8f8d5ba181a76e473763c6e26a2df6d9244813f56f893ee
feb72e5432e7a44b7f48cdab8da2c95edcec11c2 scripted-diff: rename GetSystemTimeInSeconds to GetTimeSeconds (fanquake)
Pull request description:
This PR simply renames `GetSystemTimeInSeconds` to `GetTimeSeconds`, for uniformity amongst our time handling functions (`GetTimeMillis`, `GetTimeMicros`). I have a branch that does a chunk of `GetTime()` -> `GetSystemTimeInSeconds` (`GetTimeSeconds`) / `GetTime<T>` migration, so we can eventually remove the (2 year) deprecated `GetTime()`.
However, splitting this off and doing the renaming first while the number of `GetSystemTimeInSeconds` instances is small seems worthwhile.
ACKs for top commit:
practicalswift:
cr ACK feb72e5432e7a44b7f48cdab8da2c95edcec11c2: patch looks correct
promag:
Code review ACK feb72e5432e7a44b7f48cdab8da2c95edcec11c2.
Tree-SHA512: e2ac30be9cbcd77b70c9f74bef820b558945d0fcc6f3dc59fde68a18d08a7d36f42088b804ffe7c03478c8db048615b4c4aa65a3d8d9f5d717d59b58c99f1c54
13650fe2e527bf0cf5d977bf5f3f1563b853ecdc [policy] detect unsorted packages (glozow)
9ef643e21b44f99f4bce54077788d0ad4d81f7cd [doc] add release note for package testmempoolaccept (glozow)
c4259f4b7ee23ef6e0ec82c5d5b9dfa9cadd5bed [test] functional test for packages in RPCs (glozow)
9ede34a6f20378e86c5289ebd20dd394a5915123 [rpc] allow multiple txns in testmempoolaccept (glozow)
ae8e6df709ff3d52b8e9918e09cacb64f83ae379 [policy] limit package sizes (glozow)
c9e1a26d1f17c8b98632b7796ffa8f8788b5a83c [fuzz] add ProcessNewPackage call in tx_pool fuzzer (glozow)
363e3d916cc036488783bb4bdcfdd3665aecf711 [test] unit tests for ProcessNewPackage (glozow)
cd9a11ac96c01e200d0086b2f011f4a614f5a705 [test] make submit optional in CreateValidMempoolTransaction (glozow)
2ef187941db439c5b3e529f08b6ab153ff061fc5 [validation] package validation for test accepts (glozow)
578148ded62828a9820398165c41670f4dbb523d [validation] explicit Success/Failure ctors for MempoolAcceptResult (glozow)
b88d77aec5e7bef5305a668d15031351c0548b4d [policy] Define packages (glozow)
249f43f3cc52b0ffdf2c47aad95ba9d195f6a45e [refactor] add option to disable RBF (glozow)
897e348f5987eadd8559981a973c045c471b3ad8 [coins/mempool] extend CCoinsViewMemPool to track temporary coins (glozow)
42cf8b25df07c45562b7210e0e15c3fd5edb2c11 [validation] make CheckSequenceLocks context-free (glozow)
Pull request description:
This PR enables validation dry-runs of packages through the `testmempoolaccept` RPC. The expectation is that the results returned from `testmempoolaccept` are what you'd get from test-then-submitting each transaction individually, in that order (this means the package is expected to be sorted in topological order, for now at least). The validation is also atomic: in the case of failure, it immediately halts and may return "unfinished" `MempoolAcceptResult`s for transactions that weren't fully validated. The API for 1 transaction stays the same.
**Motivation:**
- This allows you to test validity for transaction chains (e.g. with multiple spending paths and where you don't want to broadcast yet); closes#18480.
- It's also a first step towards package validation in a minimally invasive way.
- The RPC commit happens to close#21074 by clarifying the "allowed" key.
There are a few added restrictions on the packages, mostly to simplify the logic for areas that aren't critical to main package use cases:
- No package can have conflicts, i.e. none of them can spend the same inputs, even if it would be a valid BIP125 replacement.
- The package cannot conflict with the mempool, i.e. RBF is disabled.
- The total count of the package cannot exceed 25 (the default descendant count limit), and total size cannot exceed 101KvB (the default descendant size limit).
If you're looking for review comments and github isn't loading them, I have a gist compiling some topics of discussion [here](https://gist.github.com/glozow/c3acaf161c95bba491fce31585b2aaf7)
ACKs for top commit:
laanwj:
Code review re-ACK 13650fe2e527bf0cf5d977bf5f3f1563b853ecdc
jnewbery:
Code review ACK 13650fe2e527bf0cf5d977bf5f3f1563b853ecdc
ariard:
ACK 13650fe
Tree-SHA512: 8c5cbfa91a6c714e1c8710bb281d5ff1c5af36741872a7c5df6b24874d6272b4a09f816cb8a4c7de33ef8e1c2a2c252c0df5105b7802f70bc6ff821ed7cc1a2f
Pass in chainman instead of prev_block so that we can enforce the
block.hashPrevBlock refers to prev_block invariant in the function
itself.
We should probably rethink BlockAssembler's API and somehow include
commitment regeneration functionality in there. Something like a variant
of CreateNewBlock that takes in a std::vector<TxRef> and return a CBlock
instead of CBlockTemplate. That could avoid reaching for
LookupBlockIndex at all.
66545da2008cd9e806e41b74522ded259cd64f86 Remove support for double serialization (Pieter Wuille)
fff1cae43af959a601cf2558cb3c77f3c2b1aa80 Convert uses of double-serialization to {En,De}codeDouble (Pieter Wuille)
afd964d70b6f7583ecf89c380f80db07f5b66a60 Convert existing float encoding tests (Pieter Wuille)
bda33f98e2f32f2411fb0a8f5fb4f0a32abdf7d4 Add unit tests for serfloat module (Pieter Wuille)
2be4cd94f4c7d92a4287971233a20d68db81c9c9 Add platform-independent float encoder/decoder (Pieter Wuille)
e40224d0c77674348bf0a518365208bc118f39a4 Remove unused float serialization (MarcoFalke)
Pull request description:
Based on #21981.
This adds a software-based platform-independent float/double encoder/decoder (platform independent in the sense that it only uses arithmetic and library calls, but never inspects the binary representation). This should strengthen our guarantee that encoded float/double values are portable across platforms. It then removes the functionality to serialize doubles from serialize.h, and replaces its only (non-test) use for fee estimation data serialization with the software encoder.
At least on x86/ARM, the only difference should be how certain NaN values are encoded/decoded (but not *whether* they are NaN or not).
It comes with tests that verify on is_iec559 platforms (which are the only ones we support, at least for now) that the serialized bytes exactly match the binary representation of floats in memory (for non-NaN).
ACKs for top commit:
laanwj:
Code review re-ACK 66545da2008cd9e806e41b74522ded259cd64f86
practicalswift:
cr re-ACK 66545da2008cd9e806e41b74522ded259cd64f86
Tree-SHA512: 62ad9adc26e28707b2eb12a919feefd4fd10cf9032652dbb1ca1cc97638ac21de89e240858e80d293d5112685c623e58affa3d316a9783ff0e6d291977a141f5
When building for Android, _GNU_SOURCE will be defined, but it doesn't
actually have the fopencookie() function, or define the
cookie_io_functions_t type.
For now just skip trying to use it if we are building for Android.
Should fix#22062.
Maximum number of transactions allowed in a package is 25, equal to the
default mempool descendant limit: if a package has more transactions
than this, either it would fail default mempool descendant limit or the
transactions don't all have a dependency relationship (but then they
shouldn't be in a package together). Same rationale for 101KvB virtual
size package limit.
Note that these policies are only used in test accepts so far.
This allows us to easily create transaction chains for package
validation. We don't test_accept if submit=false because we want to be
able to make transactions that wouldn't pass ATMP (i.e. a child
transaction in a package would fail due to missing inputs).
0829516d1f3868c1c2ba507feee718325d81e329 [refactor] Remove unused ForEachNodeThen() template (John Newbery)
09cc66c00e1d5fabe11ffcc32cad060e6b483b20 scripted-diff: rename address relay fields (John Newbery)
76568a3351418c878d30ba0373cf76988f93f90e [net processing] Move addr relay data and logic into net processing (John Newbery)
caba7ae8a505a4b4680a9d7618f65c4e8579a1e2 [net processing] Make RelayAddress() a member function of PeerManagerImpl (John Newbery)
86acc9646968213aaa4408635915b1bfd75a10c9 [net processing] Take NodeId instead of CNode* as originator for RelayAddress() (John Newbery)
Pull request description:
This continues the work of moving application layer data into net_processing, by moving all addr data into the new Peer object added in #19607.
For motivation, see #19398.
ACKs for top commit:
laanwj:
Code review ACK 0829516d1f3868c1c2ba507feee718325d81e329
mzumsande:
ACK 0829516d1f3868c1c2ba507feee718325d81e329, reviewed the code and ran tests.
sipa:
utACK 0829516d1f3868c1c2ba507feee718325d81e329
hebasto:
re-ACK 0829516d1f3868c1c2ba507feee718325d81e329
Tree-SHA512: efe0410fac288637f203eb37d1999910791e345872d37e1bd5cde50e25bb3cb1c369ab86b3a166ffd5e06ee72e4508aa2c46d658be6a54e20b4f220d2f57d0a6
fafd121026c4f1e25d498983e4f88c119516552b refactor: Make CFeeRate constructor architecture-independent (MarcoFalke)
Pull request description:
Currently the constructor is architecture dependent. This is confusing for several reasons:
* It is impossible to create a transaction larger than the max value of `uint32_t`, so a 64-bit `size_t` is not needed
* Policy (and consensus) code should be arch-independent
* The current code will print spurious compile errors when compiled on 32-bit systems:
```
policy/feerate.cpp:23:22: warning: result of comparison of constant 9223372036854775807 with expression of type 'size_t' (aka 'unsigned int') is always true [-Wtautological-constant-out-of-range-compare]
assert(nBytes_ <= uint64_t(std::numeric_limits<int64_t>::max()));
```
Fix all issues by making it arch-independent. Also, fix `{}` style according to dev notes.
ACKs for top commit:
theStack:
re-ACK fafd121026c4f1e25d498983e4f88c119516552b
promag:
Code review ACK fafd121026c4f1e25d498983e4f88c119516552b.
Tree-SHA512: e16f75bad9ee8088b87e873906d9b5633449417a6996a226a2f37d33a2b7d4f2fd91df68998a77e52163de20b40c57fadabe7fe3502e599cbb98494178591833
