SelectorParams mempool policy

[aagbotemi]

Description

Add a validated builder interface for SelectorParams that catches dust outputs, zero-value OP_RETURNs, oversized OP_RETURN aggregates, and non-standard script types at construction time. Add a negative-fee guard and a max standard tx weight to Selection::create_psbt.

Notes to the reviewers

• OP_RETURN aggregate cap. Hardcoded at 100,000 bytes to catch obvious construction mistakes.
• Standard script types. is_standard_script checks each type explicitly (P2PK, P2PKH, P2SH, P2WPKH, P2WSH, P2TR, OP_RETURN). P2A is matched via a raw byte comparison in is_p2a.
• Renamed change_dust_relay_feerate to dust_relay_feerate. The field applies to all outputs, not just change.

Closes #41
Closes #49

Changelog notice

• Added SelectorParamsBuilder with chainable setters and two methods: build (runs check_standardness) and build_unchecked (skips validation).
• Added SelectorParamsError for builder validation failures.
• Added MAX_OP_RETURN_BYTES const (100,000-byte sanity ceiling).
• Renamed SelectorParams::change_dust_relay_feerate to dust_relay_feerate.
• Selection::create_psbt now rejects transactions where outputs exceed inputs (CreatePsbtError::NegativeFee) or where the weight exceeds the standard 400,000 WU limit (CreatePsbtError::MaxStandardTxWeightExceeded).

Checklists

• I followed the contribution guidelines
• I've added tests for the new feature
• I've added docs for the new feature
• I'm linking the issue being fixed by this PR

[noahjoeris]

I think the policy checks should be integrated more directly into the normal transaction-building flow.
Right now the caller has to remember to do a separate post-build check:

let policy = MempoolPolicy { tip_height, tip_mtp };
policy.check_all(&selection, &tx)?;

This is easy for callers to forget. It also asks them to provide both a Selection and a Transaction, even though the transaction should be derived from Selection + PsbtParams. A checked path should instead construct the transaction itself and validate exactly the transaction the crate is going to return.

Suggested shape:

• Output checks in SelectorParamsBuilder

  • build() could validate against a default policy.
  • build_with_policy(policy) could validate against a custom MempoolPolicy.
  • For unchecked, e.g. build_unchecked()

• Final transaction checks should happen in Selection.

  • create_psbt(...) defaults to e.g. Core v30
  • Add create_psbt_with_policy(policy) and create_psbt_unchecked()
  • This avoids a public check_all(&Selection, &Transaction) API and makes SelectionTxMismatch unnecessary

• So I think we should have some form of abstraction/config over policy.

  • No hardcoded policy values but make them part of MempoolPolicy struct.
  • Also chain-tip data is an unrelated runtime input that should be passed alongside, not embedded
  • Use MempoolPolicy::bitcoin_core_v30() as the default, for example.
  • Users should be able to pass a different policy if their node has different relay settings.
  • Then this policy can be passed into both SelectorParamsBuilder::build_with_policy(...) and Selection::create_psbt_with_policy(...).

Let me know what you think.

[aagbotemi]

Thank you for the review. I agree with all of this. The two-layer split made sense when I wrote it, but you're right that the manual policy.check_all(&selection, &tx) step is invisible at the type level. Nothing prompts the caller to run it, and accepting both a Selection and a Transaction lets the two drift. I will fold the validation into construction and have create_psbt_with_policy build the tx it validates.

[noahjoeris]

Nice, thanks for updating 🔥

I’d wait now for a Concept ACK on this direction by @ValuedMammal before spending more time on details.

• One API point: I’d prefer to keep create_psbt(...). It could either preserve the old unchecked behavior for compatibility, or become checked by default as create_psbt(params, tip) using MempoolPolicy::default(). I think we should discuss that explicitly. Right now most examples moved to create_psbt_unchecked(...), which makes the unchecked path look like the normal path.
• Also, MempoolPolicy only models part of Core v30 policy for now, so we should expect it to grow as more checks are added.

[ValuedMammal]

I did a first pass. Overall I like the idea and it appears to be well implemented. Biggest concern is the long term API stability as policies change and grow outdated.

Pros

• Conceptually I like the separation between standardness checks and post selection checks
• Certainly in favor of consensus-level validation to catch obvious mistakes in tx construction (e.g. negative fee)
• And maybe a subset of policy/standardness checks
• Important to differentiate tx building validation, bitcoin consensus, and local node policy
• From a maintenance point of view I would take a light touch approach to policy/standardness since it will likely be biased and incomplete

Cons

• MempoolPolicy::default() is going to be a maintenance liability since the defaults will inevitably change over time
• MempoolPolicy as a name is potentially misleading. We don't guarantee that if your tx passes MempoolPolicy, it will be accepted to mempools
• NegativeFee error is good for guarding against obviously invalid txs, but not exactly a policy preference

[ValuedMammal]

We could collapse the segwit/taproot conditions to script.is_witness_program(), which should also cover pay-to-anchor?

[ValuedMammal]

Confused why the dust-feerate of the params would override the MempoolPolicy when we're supposed to be validating the specified params against a chosen policy.

[aagbotemi]

This will be removed in the next push, along with MempoolPolicy itself.

[aagbotemi]

I did a first pass. Overall I like the idea and it appears to be well implemented. Biggest concern is the long term API stability as policies change and grow outdated.

Pros

• Conceptually I like the separation between standardness checks and post selection checks
• Certainly in favor of consensus-level validation to catch obvious mistakes in tx construction (e.g. negative fee)
• And maybe a subset of policy/standardness checks
• Important to differentiate tx building validation, bitcoin consensus, and local node policy
• From a maintenance point of view I would take a light touch approach to policy/standardness since it will likely be biased and incomplete

Cons

• MempoolPolicy::default() is going to be a maintenance liability since the defaults will inevitably change over time
• MempoolPolicy as a name is potentially misleading. We don't guarantee that if your tx passes MempoolPolicy, it will be accepted to mempools
• NegativeFee error is good for guarding against obviously invalid txs, but not exactly a policy preference

Thank you for the review. I overshot the scope. Re-reading the original TODO, this is where I got the misconception - "error on anything that doesn't satisfy mempool policy". I took that literally and tried to build toward it.

I can see that this implementation was tending toward tx broadcast preparation, which is outside the scope of bdk-tx as a tx construction library. Going forward I will narrow this PR to tx construction correctness. I'll remove MempoolPolicy, ChainTip, and the related post-selection policy functionality.

[evanlinjin]

MempoolPolicy::default() is going to be a maintenance liability since the defaults will inevitably change over time.

Maybe instead of default, we just have constructors which construct MempoolPolicy of certain Bitcoin core versions

[noahjoeris]

Thanks for updating again! 🔥 I left some comments.

Could you outline where these checks come from and why we picked this subset? Are there other checks we are intentionally leaving out?

[noahjoeris]

With this we don't have an unchecked path anymore for non standard txs right?

Also create_psbt is getting quite big. Should we add helper functions for the checks (weight, negativefee)?

[aagbotemi]

We've only covered weight and output-side checks.
Helper functions have been added.

[noahjoeris]

I'm not sure script.is_witness_program() is correct here. It allows scripts in the range of 2-40 bytes length. Which could be non-standard.

Maybe it's better to add them directly.

script.is_p2wpkh()
    || script.is_p2wsh()
    || script.is_p2tr()
    || script.is_p2a()

[aagbotemi]

script.is_p2a() is not available yet, so i used a local helper

[noahjoeris]

I think is_op_return() only checks the first byte which might not be enough.
e.g. OP_RETURN OP_CHECKSIG could pass the check

[aagbotemi]

Thanks for the catch. I've updated

[aagbotemi]

Maybe instead of default, we just have constructors which construct MempoolPolicy of certain Bitcoin core versions

The whole MempoolPolicy struct, ChainTip, and the post-selection validation path are being removed. bdk-tx is a tx construction library, not a broadcast-preparation library and versioning Core's relay policy here is the wrong layer for that responsibility.

Could you outline where these checks come from and why we picked this subset? Are there other checks we are intentionally leaving out?

After @ValuedMammal's review I've narrowed the scope significantly. The checks agreed upon will be construction invariants pre-selection, which are drawn from Bitcoin Core's IsStandardTx, narrowed to checks that hold regardless of which node or Core version you're targeting. Everything relay-readiness shaped (allowed_versions, locktime, min-relay feerate, witness stack limits, ChainTip-dependent checks) is out of scope for this library as they are transaction preparedness library.

[evanlinjin]

Is this even possible?

[evanlinjin]

check_negative_fee is unreachable since the coin selector already errors when there is insufficient funds.

This is essentially an internal logic check exposed as an external API. We should instead just have an assert statement.

[evanlinjin]

Approach NACK

I'd like to push back on the overall direction here. Mempool policy is being baked into SelectorParams (and into module-level constants), but mempool policy is not a constant — it varies by Bitcoin Core version (P2A became standard in v28, OP_RETURN policy changed materially in v30), by node implementation (Knots, btcd), and by node operator tuning (-dustrelayfee, -datacarriersize, -maxmempool). Encoding one frozen-in-time snapshot inside the selector hides that variation and makes it untunable.

Concretely, the current PR scatters policy in several places:

• dust_relay_feerate is a field on SelectorParams.
• MAX_OP_RETURN_BYTES is a pub const in selector.rs.
• MAX_STANDARD_TX_WEIGHT is hardcoded inside Selection::check_max_standard_weight.
• The standard-script allowlist is hardcoded in is_standard_script.

These are all the same kind of thing, presented as if there's one true answer.

I'd prefer a dedicated StandardnessPolicy type that owns this surface:

#[non_exhaustive]
pub struct StandardnessPolicy {
    pub dust_relay_feerate: FeeRate,
    pub max_standard_tx_weight: Weight,
    pub max_op_return_bytes: usize,
    // ...standard output kinds, datacarrier flags, etc.
}

impl StandardnessPolicy {
    pub fn bitcoin_core_v28() -> Self { ... }
    pub fn bitcoin_core_v29() -> Self { ... }
    pub fn bitcoin_core_v30() -> Self { ... }

    pub fn check_output(&self, _: &Output)     -> Result<(), _>;
    pub fn check_input(&self,  _: &Input)      -> Result<(), _>;
    pub fn check_tx(&self,     _: &TxTemplate) -> Result<(), _>;
}

The name maps 1:1 to Bitcoin Core's IsStandardTx — these are exactly the rules that function checks. RelayPolicy or MempoolPolicy would be fuzzier, since "relay" / "mempool" also cover stateful concerns (min-relay-fee, RBF rules, ancestor limits) that explicitly do not belong here (see point 4 below).

SelectorParams would drop dust_relay_feerate and the standardness checks. Callers can validate outputs pre-selection (policy.check_output), the full shape post-template (policy.check_tx), or skip validation entirely if they're deliberately producing non-standard txs. The selector itself stays policy-agnostic.

A few specific things worth pinning down before any rework:

1. Change-output dust threshold. to_cs_change_policy currently consumes dust_relay_feerate directly. If policy moves out, either pass &StandardnessPolicy at call time or keep just the feerate field on SelectorParams. I'd lean toward passing the policy — keeps SelectorParams clean.

2. check_input scope. The standard prevout script type is already covered by checking outputs we constructed. The remaining input-side rules are mostly satisfaction-size limits (scriptSig ≤ 1650 B, P2WSH script ≤ 3600 B, taproot stack item sizes). Worth being concrete in the design — otherwise it's a method that's hard to specify.

3. Block on #73. check_tx(&TxTemplate) is the right surface but TxTemplate doesn't exist on master yet. Land #73 first; that also gives a natural place for a fluent tx_template.check_against(&policy) step.

4. Scope ceiling. Be explicit that this models static standardness, not full mempool acceptance. Ancestor/descendant counts, RBF rules 2/3/4, and mempool-min-fee are stateful and don't belong here — the name StandardnessPolicy makes that boundary self-evident.

One note on the existing Selection::create_psbt checks: check_max_standard_weight is pure policy and should move into StandardnessPolicy::check_tx. check_negative_fee is a correctness invariant rather than policy — that one can stay where it is.

[aagbotemi]

@evanlinjin Thanks for the review. I want to push back on the architectural direction, though I agree with some of the observations.

bdk-tx is a transaction construction library. The checks in this PR are deliberately minimal construction-bug guards. The four locations you flagged (dust_relay_feerate, MAX_OP_RETURN_BYTES, MAX_STANDARD_TX_WEIGHT, is_standard_script) are a fixed floor of sanity checks the library always enforces and not a partial mempool policy.

Following earlier discussion with @ValuedMammal , I think it's okay for configurable standardness/relay validation to belong in a separate library scoped to that concern. For reference, this is the design notes, a tx preparedness library will have a fundamentally different maintenance contract like tracking Core releases, node implementations, which are exactly the burden you identified.

Earlier reviews on this PR pushed back on MempoolPolicy::default() and the version-tracking burden it implied. The proposed StandardnessPolicy::bitcoin_core_v28/v29/v30() constructors relocate that burden but don't eliminate it because every future Core release that changes relay rules still forces a decision in bdk-tx.

Once StandardnessPolicy exists, every future addition (check_input, min_relay_feerate, ancestor/descendant accounting) may become "we already have a policy type, why not add this too?", which is exactly why the abstraction belongs in bdk_mempool, not bdk-tx.

Some of the points you raised:

1. Change-output dust threshold Stays on SelectorParams because to_cs_change_policy needs it to compute change min_value.
2. check_input scope. Agreed it's hard to specify cleanly. Out of scope here, belongs in bdk_mempool if anywhere.
3. Scope ceiling. Agreed. I'll add a module-level doc stating bdk-tx does output-side construction-bug checks only.

What I suggest we change

• Move check_max_standard_weight to opt-in. You're right that this is policy, not invariant. I'll move it to a separate Selection::check_standardness method called explicitly.
• check_negative_fee stays where it is. Construction invariant, not policy.
• Tighten doc comments on check_standardness and the weight check to state what they are not doing.

[evanlinjin]

@aagbotemi have you curated this AI generated response before posting it?

Standardness policy by definition is a subset of mempool policy.

The response is self contradictory. I'm not even sure how to respond to it.

[evanlinjin]

Looking at the bdk_mempool plan, I propose splitting the logic up into the following crates:

• bdk_standardness - pure IsStandardTx.
• bdk_mempool - has awareness of mempool snapshot, total UTXO set to determine whether the candidate tx will be accepted. Depends on bdk_standardness.
• bdk_tx - will use bdk_standardness for sanity checks.

I think it makes sense to turn this repo into a cargo workspace to contain the above. cc. @ValuedMammal @nymius

In regards to what remains in this PR (if we are going forward with this proposal):

• check_negative_fee is unreachable and should be removed.
• Everything else is IsStandardTx checks and should be implemented in bdk_standardness.

[aagbotemi]

@aagbotemi have you curated this AI generated response before posting it?

@evanlinjin the summary of the response I gave is trying to explain that the bdk-tx is a transaction building library and trying to stay in that scope. I understand that Standardness Policy is a subset of Mempool Policy, but only pointing out maintenance burden of having v28/v29/v30/v31 and so on, for a tx building library.

I think it makes sense to turn this repo into a cargo workspace to contain the above.

I think your proposal to split into a cargo workspace makes sense, such that the logic will be shared into different crates.

[ValuedMammal]

I agree the Bitcoin Core version specific policies are overkill for right now, and we should instead focus on structural validity. Not sure what that leaves to be implemented since we've ruled out the negative fee possibility. While some of these technically fall under standardness I'm not against the idea of including basic sanity checks that we define and aren't likely to change very often. Of course these are guardrails and not hard and fast rules.

• is standard spk
• min/max tx weight
• output below dust
• max feerate
• max burn amount

[evanlinjin]

I agree the Bitcoin Core version specific policies are overkill for right now

What do you mean by overkill?

we should instead focus on structural validity. Not sure what that leaves to be implemented since we've ruled out the negative fee possibility.

Yes, bdk_tx should never create structurally invalid transactions that can never enter a block.

While some of these technically fall under standardness I'm not against the idea of including basic sanity checks that we define and aren't likely to change very often. Of course these are guardrails and not hard and fast rules.

There are two concerns here. One is IsStandardTx, the other is UX related.

• Standardness: Is standard spk?, min/max tx weight, output below dust, etc.
• UX: Max feerate, Max burn amount, Fee/send-amount ratio (maybe).

Both of these concerns can be categorized under "basic sanity checks". However, I think it makes sense for the UX checks to exist in this crate (if we decide to have them). The values are ours to define, they don't change with Core releases.

---

Let me clarify my reasoning for the proposed direction.

Why did I suggest a separate bdk_standardness crate?

Both bdk_mempool and bdk_tx (with basic sanity checks) need standardness checks. It's best to implement these somewhere accessible to both.

If we implement standardness checks in bdk_tx, we either need to make bdk_mempool depend on bdk_tx, or duplicate those checks in bdk_mempool. Both are non-ideal:

• Wrong dependency direction. A mempool checker importing a tx-construction lib is upside down.
• Duplication drifts. Two implementations of the same policy guarantee that whichever is wrong at any given moment is a bug waiting to bite a specific caller.

bdk_tx would pub use bdk_standardness as standardness;, so user-facing call sites are identical to having the checks in bdk_tx directly. The split is for the dependency DAG, not for users.

The other option is to have the standardness checks in bdk_mempool and bdk_tx will just depend on that. However, that might be worse than having bdk_standardness because that pulls in the UTXO & mempool machinery that the majority of callers will not need.

Why StandardnessPolicy as a struct?

• Standardness policy is not constant — changes happen relatively often and different node implementations have different policies. Empirically, Bitcoin Core ships standardness changes ~2-4 times per year, and essentially every major release touches something. Recent examples that matter for tx construction:

  • v28.0 (Oct 2024): P2A (BIP433) made standard, TRUC v3 transactions made standard, full-RBF made the default.
  • v30.0 (Oct 2025): datacarriersize default raised 83 → 100,000 bytes; multiple OP_RETURNs per tx allowed; per-tx legacy sigop cap added.

  Concretely, the PR-as-written hardcodes MAX_OP_RETURN_BYTES = 100_000. That value is the v30.0 default; the prior default was 83 bytes. The PR is already implicitly tracking the v30.0 default without naming the version. Anyone depending on bdk_tx today gets v30.0 OP_RETURN semantics whether they intended to or not. A StandardnessPolicy struct makes that choice explicit (or at minimum, introspectable).

  Across node implementations the divergence is larger: Bitcoin Knots defaults datacarriersize to 42 bytes (vs Core's 100,000) and applies stricter relay filtering generally; Libre Relay deliberately relays consensus-valid-but-non-standard transactions; alternative implementations (btcd, etc.) carry their own policy code. A struct lets callers target a specific deployment instead of bdk_tx's choice.

• Separation of concern. Having a separate StandardnessPolicy struct tells us explicitly what it is without relying on the docs. It's also introspectable at runtime — "what policy was applied" becomes a value you can inspect, log, or compare against the actual relaying node's policy when debugging a rejected broadcast, instead of a source-code question.

• Rare, but it's possible that not everybody wants to create a standard transaction. StandardnessPolicy allows the caller to define its own. Real cases: protocol R&D, high-data-carrier applications, miner direct-to-block, CPFP sponsors in package relay. With a StandardnessPolicy they pass a relaxed or custom-tuned policy and still get bdk_tx's construction help. Without it, the only escape hatch is build_unchecked — all-or-nothing.