Age | Commit message (Collapse) | Author |
|
Move amount.h to consensus/amount.h.
Renames, adds missing and removes uneeded includes.
|
|
|
|
Document a few non-obvious things and delete no-longer-relevant comments
(e.g. about taking a lock that we're already holding).
No change in behavior.
|
|
|
|
|
|
"Fee Delta" is already a term used for prioritizing transactions:
modified = base fees + delta
Here, delta also means the difference between original and modified replacement fees:
nDeltaFees = (original_base + original_delta) - (replacement_base + replacement_delta)
This is insanely confusing. Also, since mempool is no longer a member of a
class (MemPoolAccept.m_pool), the "m" prefix is unnecessary. The rest are
clarity/style-focused changes to already-touched lines.
-BEGIN VERIFY SCRIPT-
ren() { sed -i "s/\<$1\>/$2/g" src/policy/rbf* ; }
ren nDeltaFees additional_fees
ren m_pool pool
ren nSize replacement_vsize
ren nModifiedFees replacement_fees
ren nConflictingFees original_fees
ren oldFeeRate original_feerate
ren newFeeRate replacement_feerate
ren setAncestors ancestors
ren setIterConflicting iters_conflicting
ren setConflictsParents parents_of_conflicts
ren setConflicts direct_conflicts
ren allConflicting all_conflicts
sed -i "s/ hash\b/ txid/g" src/policy/rbf*
-END VERIFY SCRIPT-
|
|
|
|
|
|
This checks that a transaction isn't trying to replace something it
supposedly depends on.
|
|
|
|
Avoids reusing err_string.
|
|
|
|
A circular dependency is added because policy now depends on txmempool and
txmempool depends on validation. It is natural for [mempool] policy to
rely on mempool; the problem is caused by txmempool depending on
validation. #22677 will resolve this.
|
|
Co-authored-by: ariard <antoine.riard@gmail.com>
|
|
|
|
testmempoolaccept
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
|
|
|
|
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.
|
|
architecture-independent
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
|
|
-BEGIN VERIFY SCRIPT-
git ls-files -- 'src' ':(exclude)src/util/system.h' ':(exclude)src/util/system.cpp' | xargs sed -i 's/GetDataDir()/gArgs.GetDataDirNet()/g';
-END VERIFY SCRIPT-
|
|
Define the Package type as an alias for a vector of transactions for now.
Add PackageValidationResult, similar to TxValidationResult and
BlockValidationResult for package-wide errors that cannot be reported
within a single transaction result, such as having too many
transactions in the package. We can update the concept of
what a package is and have different logic for packages vs lists of
transactions in the future, e.g. for package relay.
|
|
|
|
-BEGIN VERIFY SCRIPT-
sed -i 's|/kB|/kvB|g' $( git grep -l '/kB' ./src )
-END VERIFY SCRIPT-
|
|
|
|
|
|
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
|
|
|
|
fade6195b1c230edd561443637a7bde81c2594a4 Move TX_MAX_STANDARD_VERSION to policy (MarcoFalke)
Pull request description:
`primitives` should only be used for the raw datastructures (parsing and format). It is not the right place to document relay policy.
ACKs for top commit:
laanwj:
Code review ACK fade6195b1c230edd561443637a7bde81c2594a4
lontivero:
Concept ACK https://github.com/bitcoin/bitcoin/pull/20611/commits/fade6195b1c230edd561443637a7bde81c2594a4
Tree-SHA512: f809c4aecd14d7e9feaa7b50b9c0697232991eef36190cd960bcfb0ad6e20c71a4f6aab48c7747cf8a681eb14feda60c55b09a37f128673d519567224f29cd97
|
|
Also remove extraneous whitespace, should be reviewed with --ignore-all-space
|
|
An uppercase "ERROR" in the log might indicate a fatal error. Though,
all read-failures for fee_estimates.dat are non-fatal, so avoid the
"ERROR".
Before:
ERROR: CBlockPolicyEstimator::Read(): up-version (149900) fee estimate file
After:
CBlockPolicyEstimator::Read(): unable to read policy estimator data (non-fatal): up-version (149900) fee estimate file
|
|
|
|
This moves the fee_estimates file management to the CBlockPolicyEstimator
Flush() method.
Co-authored-by: John Newbery <john@johnnewbery.com>
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
|
|
2d8099c713dfd4b546150fd53c2e4f364b9009f4 Mention units of MAX_STANDARD_ policy constants (Pieter Wuille)
84e29c7c0141b52044020ec0c5dfa8a462b7e97f Mention in validation that IsWitnessStandard tests for P2TR (Pieter Wuille)
f867cbcc268a3bfaeef5510a7e40e6d3c0818b6d Clean up assets test minimizer LDFLAGS (Pieter Wuille)
ea0e78677bdbe3313f594118c500cf7784c56970 Document additional IsWitnessStandard behavior (Pieter Wuille)
6040de9a46725826330cd63cdf76e2121a18e728 Add comments on CPubKey::IsValid (Pieter Wuille)
8dbb7de67ce0a71f5fc54289c0ff048ac8dd0acc Add comments to VerifyTaprootCommitment (Pieter Wuille)
cdf900cbf26db05c7edb398ea645f1d23049d810 Document need_vin_vout_mismatch argument to make_spender (Pieter Wuille)
18246ed5f09dd078fa1410b7ec2ba4379cc5e032 Fix and improve taproot_construct comments (Pieter Wuille)
Pull request description:
Addressing some review comments raised here: https://github.com/bitcoin/bitcoin/pull/19953#pullrequestreview-512238027 and https://github.com/bitcoin/bitcoin/pull/19953#pullrequestreview-513499921
ACKs for top commit:
jonatack:
ACK 2d8099c per `git range-diff 5009159 4f10965 2d8099c`
ariard:
ACK 2d8099c, only changes are comment light improvements on IsValid/IsWitnessStandard.
Tree-SHA512: c4881546c379ea8efc7ef99a43cbf3b9cd3f9dde5fd97a07ee66f2b593c78aef0bd8784853c5c9c737b66c269241a1048bbbdd6c964a3d872efd8ba0ec410b68
|
|
|
|
|
|
Bitcoin core has a standardness rule for max satisfaction script sig size.
This PR adds to the policy header file so that it is documented along with
along policy rules. The initial reasoning that 1650 is an implicit
limit(would not reached assuming all other policy rules are being
followed) is outdated.
As we now know, bitcoin transactions can have spend conditions are more than
just signatures and there may exist p2sh transactions involving 100 byte
preimages that maybe non-standard because of this rule. Because this
rule is no longer implicit, we should explicitly document it in policy
header file
|
|
and BTC/kvB for feeRate error messages.
|
|
as requested by reviewers
|
|
|
|
This adds a `TxoutType::WITNESS_V1_TAPROOT` for P2TR outputs, and permits spending
them in standardness rules. No corresponding `CTxDestination` is added for it,
as that isn't needed until we want wallet integration. The taproot validation flags
are also enabled for mempool transactions, and standardness rules are added
(stack item size limit, no annexes).
|
|
Reported-by: practicalswift <practicalswift@users.noreply.github.com>
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
|
|
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
|
|
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
|
|
It was always passed as true, and complicates the (already complex)
logic of the function.
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
|
|
This was confusing: which one is the good one ? After testing the value
is right but not the comment, so fix it.
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
|
|
|
|
Co-authored-by: John Newbery <jonnynewbs@gmail.com>
|
|
Our policy checks for non-standard inputs depend only on the non-witness
portion of a transaction: we look up the scriptPubKey of the input being
spent from our UTXO set (which is covered by the input txid), and the p2sh
checks only rely on the scriptSig portion of the input.
Consequently it's safe to add txids of transactions that fail these checks to
the reject filter, as the witness is irrelevant to the failure. This is helpful
for any situation where we might request the transaction again via txid (either
from txid-relay peers, or if we might fetch the transaction via txid due to
parent-fetching of orphans).
Further, in preparation for future witness versions being deployed on the
network, ensure that WITNESS_UNKNOWN transactions are rejected in
AreInputsStandard(), so that transactions spending v1 (or greater) witness
outputs will fall into this category of having their txid added to the reject
filter.
|
|
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
|
|
fa32adf9dc25540ad27f5b82654c7057d7738627 scripted-diff: TxoutType C++11 scoped enum class (MarcoFalke)
fa95a694c492b267e4038674fd3f338dd215ab48 doc: Update outdated txnouttype documentation (MarcoFalke)
fa58469c770d8c935a86462634e4e8cd806aa6e3 rpc: Properly use underlying type in GetAllOutputTypes (MarcoFalke)
fa41c657022b8f99c8e6718a0e33c5838c412a0b rpc: Simplify GetAllOutputTypes with the Join helper (MarcoFalke)
Pull request description:
Non-scoped enums can accidentally and silently decay into an integral type. Also, the symbol names of the keys are exported to the surrounding (usually global) namespace.
Fix both issues by switching to an `enum class TxoutType` in a (mostly) scripted-diff.
ACKs for top commit:
practicalswift:
ACK fa32adf9dc25540ad27f5b82654c7057d7738627 -- patch looks correct
hebasto:
re-ACK fa32adf9dc25540ad27f5b82654c7057d7738627, since fa5997bd6fc82e16b597ea96e3c5c665f1f174ab (https://github.com/bitcoin/bitcoin/pull/19114#pullrequestreview-421425198) rebased only (verified with `git range-diff`).
Tree-SHA512: f42a9db47f9be89fa4bdd8d2fb05a16726286d8b12e3d87327b67d723f91c7d5a57deb4b2ddae9e1d16fee7a5f8c00828b6dc8909c5db680fc5e0a3cf07cd465
|