91d924ede1b421df31c895f4f43359e453a09ca5 Rename script/standard.{cpp/h} to script/solver.{cpp/h} (Andrew Chow)
bacdb2e208531124e85ed2d4ea2a4b508fbb5088 Clean up script/standard.{h/cpp} includes (Andrew Chow)
f3c9078b4cddec5581e52de5c216ae53984ec130 Clean up things that include script/standard.h (Andrew Chow)
8bbe257bac751859a272ddf52dc0328c1b5a1ede MOVEONLY: Move datacarrier defaults to policy.h (Andrew Chow)
7a172c76d2361fc3cdf6345590e26c79a7821672 Move CTxDestination to its own file (Andrew Chow)
145f36ec81e79d2e391847520364c2420ef0e0e8 Move Taproot{SpendData/Builder} to signingprovider.{h/cpp} (Andrew Chow)
86ea8bed5473f400f7a93fcc455393a574a2f319 Move CScriptID to script.{h/cpp} (Andrew Chow)
b81ebff0d99c45c071b999796b8ae3f0f2517b22 Remove ScriptHash from CScriptID constructor (Andrew Chow)
cba69dda3da0e4fa39cff5ce4dc81d1242fe651b Move MANDATORY_SCRIPT_VERIFY_FLAGS from script/standard.h to policy/policy.h (Anthony Towns)
Pull request description:
Some future work needs to touch things in script/standard.{h/cpp}, however it is unclear if it is safe to do so as they are included in several different places that could effect standardness and consensus. It contains a mix of policy parameters, consensus parameters, and utilities only used by the wallet. This PR breaks up the various components and renames the files to clearly separate everything.
* `CTxDestination` is moved to a new file `src/addresstype.{cpp/h}`
* `TaprootSpendData` and `TaprootBuilder` (and their utility functions and structs) are moved to `SigningProvider` as these are used only during signing.
* `CScriptID` is moved to `script/script.h` to be next to `CScript`.
* `MANDATORY_SCRIPT_VERIFY_FLAGS` is moved to `interpreter.h`
* The parameters `DEFAULT_ACCEPT_DATACARRIER` and `MAX_OP_RETURN_RELAY` are moved to `policy.h`
* `standard.{cpp/h}` is renamed to `solver.{cpp/h}` since that's all that's left in the file after the above moves
ACKs for top commit:
Sjors:
ACK 91d924ede1b421df31c895f4f43359e453a09ca5
ajtowns:
ACK 91d924ede1b421df31c895f4f43359e453a09ca5
MarcoFalke:
ACK 91d924ede1b421df31c895f4f43359e453a09ca5 😇
murchandamus:
ACK 91d924ede1b421df31c895f4f43359e453a09ca5
darosior:
Code review ACK 91d924ede1b421df31c895f4f43359e453a09ca5.
theStack:
Code-review ACK 91d924ede1b421df31c895f4f43359e453a09ca5
Tree-SHA512: d347439890c652081f6a303d99b2bde6c371c96e7f4127c5db469764a17d39981f19884679ba883e28b733fde6142351dd8288c7bc61c379b7eefe7fa7acca1a
Any identifier starting with two _, or one _ followed by a capital letter is reserved for the compiler and thus must not be used. See: https://stackoverflow.com/a/228797/7130273
-BEGIN VERIFY SCRIPT-
s() { git grep -l "$1" src | xargs sed -i "s/$1/$2/g"; }
s '__pushKV' 'pushKVEnd'
s '_EraseTx' 'EraseTxNoLock'
s '_Other' 'Other'
-END VERIFY SCRIPT-
The fs.* files are already part of the libbitcoin_util library. With the
introduction of the fs_helpers.* it makes sense to move fs.* into the
util/ directory as well.
7013da07fbcddb04abae9759767a9419ab90444c Add release note for PR#25943 (David Gumberg)
04f270b4358417fc2827b9f91717816062b1864e Add test for unspendable transactions and parameter 'maxburnamount' to sendrawtransaction. (David Gumberg)
Pull request description:
This PR adds a user configurable, zero by default parameter — `maxburnamount` — to `sendrawtransaction`. This PR makes bitcoin core reject transactions that contain unspendable outputs which exceed `maxburnamount`. closes#25899.
As a result of this PR, `sendrawtransaction` will by default block 3 kinds of transactions:
1. Those that begin with `OP_RETURN` - (datacarriers)
2. Those whose lengths exceed the script limit.
3. Those that contain invalid opcodes.
The user is able to configure a `maxburnamount` that will override this check and allow a user to send a potentially unspendable output into the mempool.
I see two legitimate use cases for this override:
1. Users that deliberately use `OP_RETURN` for datacarrier transactions that embed data into the blockchain.
2. Users that refuse to update, or are unable to update their bitcoin core client would be able to make use of new opcodes that their client doesn't know about.
ACKs for top commit:
glozow:
reACK 7013da07fbcddb04abae9759767a9419ab90444c
achow101:
re-ACK 7013da07fbcddb04abae9759767a9419ab90444c
Tree-SHA512: f786a796fb71a587d30313c96717fdf47e1106ab4ee0c16d713695e6c31ed6f6732dff6cbc91ca9841d66232166eb058f96028028e75c1507324426309ee4525
If a `submitpackage` RPC call errors due to any of the individual tx
broadcasts failing, the returned error message is supposed to contain
the number of successful broadcasts so far. Right now this is wrongly
always shown as zero. Fix this by adding the missing counting.
(Note though that the error should be really rare, as all txs have
already been submitted succesfully to the mempool.)
This value creates an extremely confusing interface as its existence is
dependent upon implementation details (whether something was submitted
on its own, etc). MempoolAcceptResult::m_effective_feerate is much more
helpful, as it always exists for submitted transactions.
47c4b1f52ab8d95d7deef83050bad49d1e3e5990 mempool: log/halt when CalculateMemPoolAncestors fails unexpectedly (stickies-v)
5481f65849313ff947f38433b1ac28285a7f7694 mempool: add AssumeCalculateMemPoolAncestors helper function (stickies-v)
f911bdfff95eba3793fffaf71a31cc8bfc6f80c9 mempool: use util::Result for CalculateMemPoolAncestors (stickies-v)
66e028f7399b6511f9b73b1cef54b6a6ac38a024 mempool: use util::Result for CalculateAncestorsAndCheckLimits (stickies-v)
Pull request description:
Upon reviewing the documentation for `CTxMemPool::CalculateMemPoolAncestors`, I noticed `setAncestors` was meant to be an `out` parameter but actually is an `in,out` parameter, as can be observed by adding `assert(setAncestors.empty());` as the first line in the function and running `make check`. This PR fixes this unexpected behaviour and introduces refactoring improvements to make intents and effects of the code more clear.
## Unexpected behaviour
This behaviour occurs only in the package acceptance path, currently only triggered by `testmempoolaccept` and `submitpackage` RPCs.
In `MemPoolAccept::AcceptMultipleTransactions()`, we first call `PreChecks()` and then `SubmitPackage()` with the same `Workspace ws` reference. `PreChecks` leaves `ws.m_ancestors` in a potentially non-empty state, before it is passed on to `MemPoolAccept::SubmitPackage`. `SubmitPackage` is the only place where `setAncestors` isn't guaranteed to be empty before calling `CalculateMemPoolAncestors`. The most straightforward fix is to just forcefully clear `setAncestors` at the beginning of CalculateMemPoolAncestors, which is done in the first bugfix commit.
## Improvements
### Return value instead of out-parameters
This PR updates the function signatures for `CTxMemPool::CalculateMemPoolAncestors` and `CTxMemPool::CalculateAncestorsAndCheckLimits` to use a `util::Result` return type and eliminate both the `setAncestors` `in,out`-parameter as well as the error string. It simplifies the code and makes the intent and effects more explicit.
### Observability
There are 7 instances where we currently call `CalculateMemPoolAncestors` without actually checking if the function succeeded because we assume that it can't fail, such as in [miner.cpp](69b10212ea/src/node/miner.cpp (L399)). This PR adds a new wrapper `AssumeCalculateMemPoolAncestors` function that logs such unexpected failures, or in case of debug builds even halts the program. It's not crucial to the objective, more of an observability improvement that seems sensible to add on here.
ACKs for top commit:
achow101:
ACK 47c4b1f52ab8d95d7deef83050bad49d1e3e5990
w0xlt:
ACK 47c4b1f52a
glozow:
ACK 47c4b1f52ab8d95d7deef83050bad49d1e3e5990
furszy:
light code review ACK 47c4b1f5
aureleoules:
ACK 47c4b1f52ab8d95d7deef83050bad49d1e3e5990
Tree-SHA512: d908dad00d1a5645eb865c4877cc0bae74b9cd3332a3641eb4a285431aef119f9fc78172d38b55c592168a73dae83242e6af3348815f7b37cbe2d448a3a58648
When CalculateMemPoolAncestors fails unexpectedly (e.g. it exceeds
ancestor/descendant limits even though we expect no limits to be applied),
add an error log entry for increased visibility. For debug builds,
the application will even halt completely since this is not supposed
to happen.
Simplifies function signatures by removing repetition of all the
ancestor/descendant limits, and increases readability by being
more verbose by naming the limits, while still reducing the LoC.
Our RBF policy is different from the rules specified in BIP125. For
example, the BIP does not mention Rule 6, and our Rule 4 uses the
(configurable) incremental relay feerate (distinct from the
minimum relay feerate). Those interested in our policy should refer to
doc/policy/mempool-replacements.md instead. These rules may also
continue to diverge with package RBF and other RBF improvements. Keep
references to the BIP125 signaling wrt sequence numbers, since that is
still correct and widely used. It is helpful to refer to this as "BIP125
signaling" since it is unambiguous and succint, especially if we have
multiple ways to signal replaceability in the future.
The rule numbers in doc/policy/mempool-replacements.md correspond
largely to those of BIP 125, so we can still refer to them like "Rule 5."
m_is_loaded/IsLoaded() doesn't actually indicate whether or not the
mempool was successfully, loaded, but rather if a load has been
attempted and did not result in a catastrophic ShutdownRequested.
-BEGIN VERIFY SCRIPT-
find_regex="\bm_is_loaded\b" \
&& git grep -l -E "$find_regex" \
| xargs sed -i -E "s@$find_regex@m_load_tried@g"
find_regex="\bIsLoaded\b" \
&& git grep -l -E "$find_regex" \
| xargs sed -i -E "s@$find_regex@GetLoadTried@g"
find_regex="\bSetIsLoaded\b" \
&& git grep -l -E "$find_regex" \
| xargs sed -i -E "s@$find_regex@SetLoadTried@g"
-END VERIFY SCRIPT-
Also introduce node::{ShouldPersistMempool,MempoolPath} helper functions
in node/mempool_persist_args.{h,cpp} which are used by non-kernel
DumpMempool callers to determine whether or not to automatically dump
the mempool and where to dump it to.
4c9666bd73645b94ae81be1faf7a0b8237dd6e99 Mention `mempoolfullrbf` in policy/mempool-replacements.md (Antoine Riard)
aae66ab43d794bdfaa2dade91760cc55861c9693 Update getmempoolinfo RPC with `mempoolfullrbf` (Antoine Riard)
3e27e317270fdc2dd02794fea9da016387699636 Introduce `mempoolfullrbf` node setting. (Antoine Riard)
Pull request description:
This is ready for review.
Recent discussions among LN devs have brought back on the surface concerns about the security of multi-party funded transactions against pinnings attacks and other mempool-based nuisances. The lack of full-rbf transaction-relay topology connected to miners open the way to cheap and naive DoS against multi-party funded transactions (e.g coinjoins, dual-funded channels, on-chain DLCs, ...) without solutions introducing an overhead cost or centralization vectors afaik . For more details, see [0].
This PR implements a simple `fullrbf` setting, where the node always allows transaction replacement, ignoring BIP125 opt-in flag. The default value of the setting stays **false**, therefore opt-in replacement is still the default Bitcoin Core replacement policy. Contrary to a previous proposal of mine and listening to feedbacks collected since then [1], I think this new setting simply offers more flexibility in a node transaction-relay policy suiting one's application requirements, without arguing a change of the default behavior.
I [posted](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-June/020557.html) on the ML to invite operators with a bitcoin application sensitive to full-rbf (e.g dual-funded LN channels service providers) or mempool researchers to join a bootstrapped full-rbf activated peers network for experimentation and learning. If people have strong opinions against the existence of such full-rbf transaction-relay network, I'm proposing to express them on the future thread.
[0] https://lists.linuxfoundation.org/pipermail/lightning-dev/2021-May/003033.html
[1] https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-June/019074.html
Follow-up suggestions :
- soft-enable opt-in RBF in the wallet : https://github.com/bitcoin/bitcoin/pull/25353#issuecomment-1154918789
- p2p discovery and additional outbound connection to full-rbf peers : https://github.com/bitcoin/bitcoin/pull/25353#issuecomment-1156044401
- match the code between RPC, wallet and mempool about disregard of inherited signaling : #22698
ACKs for top commit:
instagibbs:
reACK 4c9666bd73
glozow:
ACK 4c9666bd73645b94ae81be1faf7a0b8237dd6e99, a few nits which are non-blocking.
w0xlt:
ACK 4c9666bd73
Tree-SHA512: 9e288bf22e06a9808804e58178444ef1830c3fdd42fd8a7cd7ffb101f8f586e08b000679be407d63ca76a56f7216227b368ff630c81f3fac3243db1a1202ab1c
e866f0d0666664885d4c15c79bf59cc59975887a [functional test] submitrawpackage RPC (glozow)
fa076515b07ac4b10b2134e323bf4f56be5996a8 [rpc] add new submitpackage RPC (glozow)
Pull request description:
It would be nice for LN/wallet/app devs to test out package policy, package RBF, etc., but the only interface to do so right now is through unit tests. This PR adds a `-regtest` only RPC interface so people can test by submitting raw transaction data. It is regtest-only, as it would be unsafe/confusing to create an actual mainnet interface while package relay doesn't exist.
Note that the functional tests are there to ensure the RPC interface is working properly; they aren't for testing policy itself. See src/test/txpackage_tests.cpp.
ACKs for top commit:
t-bast:
Tested ACK against eclair e866f0d066
ariard:
Code Review ACK e866f0d0
instagibbs:
code review ACK e866f0d0666664885d4c15c79bf59cc59975887a
Tree-SHA512: 824a26b10d2240e0fd85e5dd25bf499ee3dd9ba8ef4f522533998fcf767ddded9f001f7a005fe3ab07ec95e696448484e26599803e6034ed2733125c8c376c84
d1684beabe5b738c2cc83de83e1aaef11a761b69 fees: Pass in a filepath instead of referencing gArgs (Carl Dong)
9a3d825c30e8e6118d74a4e568744cb9d03f7f5d init: Remove redundant -*mempool*, -limit* queries (Carl Dong)
6c5c60c4124293d948735756f84efc85262ea66f mempool: Use m_limit for UpdateTransactionsFromBlock (Carl Dong)
9e93b1030182eff92ef91181e17c7dd498c7e164 node/ifaces: Use existing MemPoolLimits (Carl Dong)
38af2bcf358a72b9457d370282e57f4be1c5c849 mempoolaccept: Use limits from mempool in constructor (Carl Dong)
9333427014695ac235c96d48791098168dfdc9db mempool: Introduce (still-unused) MemPoolLimits (Carl Dong)
716bb5fbd31077bbe99d11a54d6c2c250afc8085 scripted-diff: Rename anc/desc size limit vars to indicate SI unit (Carl Dong)
1ecc77321deb61b9f6888e4e10752b9d972fd26e scripted-diff: Rename DEFAULT_MEMPOOL_EXPIRY to indicate time unit (Carl Dong)
aa9141cd8185cb7ad532bc16feb9d302b05d9697 mempool: Pass in -mempoolexpiry instead of referencing gArgs (Carl Dong)
51c7a41a5eb6fcb60333812c770d80227cf7b64d init: Only determine maxmempool once (Carl Dong)
386c9472c8764738282e6d163b42e15a8feda7ea mempool: Make GetMinFee() with custom size protected (Carl Dong)
82f00de7a6a60cbc9ad0c6e1d0ffb1bc70c49af5 mempool: Pass in -maxmempool instead of referencing gArgs (Carl Dong)
f1941e8bfd2eecc478c7660434b1ebf6a64095a0 pool: Add and use MemPoolOptions, ApplyArgsManOptions (Carl Dong)
0199bd35bb44e32ee0db9b51c9d1bd7518c26f19 fuzz/rbf: Add missing TestingSetup (Carl Dong)
ccbaf546a68d6cda8ed3efd0598c0e4121b366bb scripted-diff: Rename DEFAULT_MAX_MEMPOOL_SIZE to indicate SI unit (Carl Dong)
fc02f77ca604f0221171bfde3059b34f5d0fb1cd ArgsMan: Add Get*Arg functions returning optional (Carl Dong)
Pull request description:
This is part of the `libbitcoinkernel` project: #24303, https://github.com/bitcoin/bitcoin/projects/18
-----
As mentioned in the Stage 1 Step 2 description of [the `libbitcoinkernel` project](https://github.com/bitcoin/bitcoin/issues/24303), `ArgsManager` will not be part of `libbitcoinkernel`. Therefore, it is important that we remove any dependence on `ArgsManager` by code that will be part of `libbitcoinkernel`. This is the first in a series of PRs aiming to achieve this.
This PR removes `CTxMemPool+MempoolAccept`'s dependency on `ArgsManager` by introducing a `CTxMemPool::Options` struct, which is used to specify `CTxMemPool`'s various options at construction time.
These options are:
- `-maxmempool` -> `CTxMemPool::Options::max_size`
- `-mempoolexpiry` -> `CTxMemPool::Options::expiry`
- `-limitancestorcount` -> `CTxMemPool::Options::limits::ancestor_count`
- `-limitancestorsize` -> `CTxMemPool::Options::limits::ancestor_size`
- `-limitdescendantcount` -> `CTxMemPool::Options::limits::descendant_count`
- `-limitdescendantsize` -> `CTxMemPool::Options::limits::descendant_size`
More context can be gleaned from the commit messages. The important commits are:
- 56eb479ded8bfb2ef635bb6f3b484f9d5952c70d "pool: Add and use MemPoolOptions, ApplyArgsManOptions"
- a1e08b70f3068f4e8def1c630d8f50cd54da7832 "mempool: Pass in -maxmempool instead of referencing gArgs"
- 6f4bf3ede5812b374828f08fc728ceded2f10024 "mempool: Pass in -mempoolexpiry instead of referencing gArgs"
- 5958a7fe4806599fc620ee8c1a881ca10fa2dd16 "mempool: Introduce (still-unused) MemPoolLimits"
Reviewers: Help needed in the following commits (see commit messages):
- a1e08b70f3068f4e8def1c630d8f50cd54da7832 "mempool: Pass in -maxmempool instead of referencing gArgs"
- 0695081a797e9a5d7787b78b0f8289dafcc6bff7 "node/ifaces: Use existing MemPoolLimits"
Note to Reviewers: There are perhaps an infinite number of ways to architect `CTxMemPool::Options`, the current one tries to keep it simple, usable, and flexible. I hope we don't spend too much time arguing over the design here since that's not the point. In the case that you're 100% certain that a different design is strictly better than this one in every regard, please show us a fully-implemented branch.
-----
TODO:
- [x] Use the more ergonomic `CTxMemPool::Options` where appropriate
- [x] Doxygen comments for `ApplyArgsManOptions`, `MemPoolOptions`
-----
Questions for Reviewers:
1. Should we use `std::chrono::seconds` for `CTxMemPool::Options::expiry` and `CTxMemPool::m_expiry` instead of an `int64_t`? Something else? (`std::chrono::hours`?)
2. Should I merge `CTxMemPool::Limits` inside `CTxMemPool::Options`?
ACKs for top commit:
MarcoFalke:
ACK d1684beabe5b738c2cc83de83e1aaef11a761b69 🍜
ryanofsky:
Code review ACK d1684beabe5b738c2cc83de83e1aaef11a761b69. Just minor cleanups since last review, mostly switching to brace initialization
Tree-SHA512: 2c138e52d69f61c263f1c3648f01c801338a8f576762c815f478ef5148b8b2f51e91ded5c1be915e678c0b14f6cfba894b82afec58d999d39a7bb7c914736e0b
- Store the mempool size limit (-maxmempool) in CTxMemPool as a member.
- Remove the requirement to explicitly specify a mempool size limit for
CTxMemPool::GetMinFee(...) and LimitMempoolSize(...), just use the
stored mempool size limit where possible.
- Remove all now-unnecessary instances of:
gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000
The code change in CChainState::GetCoinsCacheSizeState() is correct
since the coinscache should not repurpose "extra" mempool memory
headroom for itself if the mempool doesn't even exist.
It could be unsafe/confusing to create an actual mainnet interface while
package relay doesn't exist. However, a regtest-only interface allows
wallet/application devs to test current package policies.
Better to be explicit when it comes to sizes to avoid unintentional
bugs. We use MB and KB all over the place.
-BEGIN VERIFY SCRIPT-
find_regex="DEFAULT_MAX_MEMPOOL_SIZE" \
&& git grep -l -E "$find_regex" \
| xargs sed -i -E "s@$find_regex@\0_MB@g"
-END VERIFY SCRIPT-
fa4068b4e2192f168bb120624eca5735f0dadf6f Move minRelayTxFee to policy/settings (MacroFake)
Pull request description:
Seems a bit confusing to put policy stuff into validation, so fix that.
Also fix includes via `iwyu`.
ACKs for top commit:
ariard:
ACK fa4068b, the includes move compiles well locally.
ryanofsky:
Code review ACK fa4068b4e2192f168bb120624eca5735f0dadf6f. Make sense to move the global variable to policy/settings and the default constant to policy/policy. Ariard points out other constants that could be moved, which seems fine, but it seems like moving the global variable to be with other related global variables is more significant.
Tree-SHA512: adf9619002610d1877f3aef0a9e6115fc4c2ad64135a3e5100824c650b560c47f47ac28894c6214a50a7888355252a9f6f7cec98c23a771a1964160ef1ca77de
We add an RPC to fetch the mempool transactions spending given outpoints.
Without this RPC, application developers would need to first call
`getrawmempool` which returns a long list of `txid`, then fetch each of
these txs individually to check whether they spend the given outpoint(s).
This RPC can later be enriched to also find confirmed transactions instead
of being restricted to mempool transactions.
