13650fe2e5 [policy] detect unsorted packages (glozow)
9ef643e21b [doc] add release note for package testmempoolaccept (glozow)
c4259f4b7e [test] functional test for packages in RPCs (glozow)
9ede34a6f2 [rpc] allow multiple txns in testmempoolaccept (glozow)
ae8e6df709 [policy] limit package sizes (glozow)
c9e1a26d1f [fuzz] add ProcessNewPackage call in tx_pool fuzzer (glozow)
363e3d916c [test] unit tests for ProcessNewPackage (glozow)
cd9a11ac96 [test] make submit optional in CreateValidMempoolTransaction (glozow)
2ef187941d [validation] package validation for test accepts (glozow)
578148ded6 [validation] explicit Success/Failure ctors for MempoolAcceptResult (glozow)
b88d77aec5 [policy] Define packages (glozow)
249f43f3cc [refactor] add option to disable RBF (glozow)
897e348f59 [coins/mempool] extend CCoinsViewMemPool to track temporary coins (glozow)
42cf8b25df [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 13650fe2e5
jnewbery:
Code review ACK 13650fe2e5
ariard:
ACK 13650fe
Tree-SHA512: 8c5cbfa91a6c714e1c8710bb281d5ff1c5af36741872a7c5df6b24874d6272b4a09f816cb8a4c7de33ef8e1c2a2c252c0df5105b7802f70bc6ff821ed7cc1a2f
66545da200 Remove support for double serialization (Pieter Wuille)
fff1cae43a Convert uses of double-serialization to {En,De}codeDouble (Pieter Wuille)
afd964d70b Convert existing float encoding tests (Pieter Wuille)
bda33f98e2 Add unit tests for serfloat module (Pieter Wuille)
2be4cd94f4 Add platform-independent float encoder/decoder (Pieter Wuille)
e40224d0c7 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 66545da200
practicalswift:
cr re-ACK 66545da200
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).
0829516d1f [refactor] Remove unused ForEachNodeThen() template (John Newbery)
09cc66c00e scripted-diff: rename address relay fields (John Newbery)
76568a3351 [net processing] Move addr relay data and logic into net processing (John Newbery)
caba7ae8a5 [net processing] Make RelayAddress() a member function of PeerManagerImpl (John Newbery)
86acc96469 [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 0829516d1f
mzumsande:
ACK 0829516d1f, reviewed the code and ran tests.
sipa:
utACK 0829516d1f
hebasto:
re-ACK 0829516d1f
Tree-SHA512: efe0410fac288637f203eb37d1999910791e345872d37e1bd5cde50e25bb3cb1c369ab86b3a166ffd5e06ee72e4508aa2c46d658be6a54e20b4f220d2f57d0a6
fafd121026 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 fafd121026
promag:
Code review ACK fafd121026.
Tree-SHA512: e16f75bad9ee8088b87e873906d9b5633449417a6996a226a2f37d33a2b7d4f2fd91df68998a77e52163de20b40c57fadabe7fe3502e599cbb98494178591833
bbbb51877a fuzz: Speed up transaction fuzz target (MarcoFalke)
Pull request description:
`hashBlock` and `include_addresses` are orthogonal, so no need to do an exhaustive "search".
Might fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34491
ACKs for top commit:
practicalswift:
cr ACK bbbb51877a: patch looks correct, and `TxToUniv` surprisingly wide in the `transaction_fuzz_target` flame graph! Putting it on a diet makes sense.
Tree-SHA512: 1e7c30c7fecf96364a9a1597c0a22139389fdeb67db59f3c2c6fc088196e3332877b2865991a957980d542f99a2f48cc066dd7cc16c695a5113190fe06205089
393992b049 fuzz: Terminate immediately if a fuzzing harness ever tries to create a TCP socket (belt and suspenders) (practicalswift)
Pull request description:
Terminate immediately if a fuzzing harness ever to create a TCP socket (belt and suspenders).
Obviously this _should_ never happen, but if it _does_ happen we want immediate termination instead of a TCP socket :)
ACKs for top commit:
MarcoFalke:
ACK 393992b049
Tree-SHA512: 5bbff1f7e9a58b3eae24f742b7daf3fc870424c985f29bed5931e47a708d9c0984bfd8762f43658cffa9c69d32f86d56deb48bc7e43821e3398052174b6a160e
Allow CheckSequenceLocks to use heights and coins from any CoinsView and
CBlockIndex provided. This means that CheckSequenceLocks() doesn't need
to hold the mempool lock or cs_main. The caller is responsible for
ensuring the CoinsView and CBlockIndex are consistent before passing
them in. The typical usage is still to create a CCoinsViewMemPool from
the mempool and grab the CBlockIndex from the chainstate tip.
ce6bca88e8 doc: release note for getnodeaddresses by network (Jon Atack)
3f89c0e990 test: improve getnodeaddresses coverage, test by network (Jon Atack)
6c98c09991 rpc: enable filtering getnodeaddresses by network (Jon Atack)
80ba294854 p2p: allow CConnman::GetAddresses() by network, add doxygen (Jon Atack)
a49f3ddbba p2p: allow CAddrMan::GetAddr() by network, add doxygen (Jon Atack)
c38981e748 p2p: pull time call out of loop in CAddrMan::GetAddr_() (João Barbosa)
d35ddca91e p2p: enable CAddrMan::GetAddr_() by network, add doxygen (Jon Atack)
Pull request description:
This patch allows passing a network argument to CAddrMan::GetAddr(), CConnman::GetAddresses(), and rpc getnodeaddresses to return only addresses of that network.
It also contains a performance optimisation by promag.
ACKs for top commit:
laanwj:
Code review and lightly tested ACK ce6bca88e8
vasild:
ACK ce6bca88e8
Tree-SHA512: 40e700d97091248429c73cbc0639a1f03ab7288e636a7b9026ad253e9708253c6b2ec98e7d9fb2d56136c0f762313dd648915ac98d723ee330d713813a43f99d
e286cd0d7b net: flag relevant Sock methods with [[nodiscard]] (Vasil Dimov)
Pull request description:
Flag relevant Sock methods with `[[nodiscard]]` to avoid issues like the one fixed in https://github.com/bitcoin/bitcoin/pull/21631.
ACKs for top commit:
practicalswift:
cr ACK e286cd0d7b: the only changes made are additions of `[[nodiscard]]` and `(void)` where appropriate
laanwj:
Code review ACK e286cd0d7b
Tree-SHA512: addc361968d24912bb625b42f4db557791556bf0ffad818252a89a32d76ac22758ec70f8282dcfbfd77eebec20a8e6bb7557c8ed08d50a58de95378c34955973
7075f604e8 scripted-diff: update noban documentation in net_processing.cpp (Jon Atack)
a95540cf43 scripted-diff: rename NetPermissionFlags enumerators (Jon Atack)
810d0929c1 p2p, refactor: make NetPermissionFlags a uint32 enum class (Jon Atack)
7b55a94497 p2p: NetPermissions::HasFlag() pass flags param by value (Jon Atack)
91f6e6e6d1 scripted-diff: add NetPermissionFlags scopes where not already present (Jon Atack)
Pull request description:
While reviewing #20196, I noticed the `NetPermissionFlags` enums are frequently called as if they were scoped, yet are still global. This patch upgrades `NetPermissionFlags` to a scoped class enum and updates the enumerator naming, similarly to #19771. See https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#enum-enumerations for more info.
This change would eliminate the class of bugs like https://github.com/bitcoin/bitcoin/pull/20196#discussion_r610770148 and #21644, as only defined operations on the flags would compile.
ACKs for top commit:
laanwj:
Code review ACK 7075f604e8
vasild:
ACK 7075f604e8
Tree-SHA512: 7fcea66ee499f059efc78c934b5f729b3c8573fe304dee2c27c837c2f662b89324790568246d75b2a574cf9f059b42d3551d928996862f4358055eb43521e6f4
If a scope id is provided, return it back in the string representation.
Also bring back the test. Closes#21982.
Co-authored-by: Jon Atack <jon@atack.com>
