Fix: TxOut::minimal_non_dust and Script::dust_value #2255

junderw · 2023-12-06T22:00:23Z

TxOut::minimal_non_dust has 3 problems.

There is an invisible dependency on Bitcoin Core's default minrelaytxfee value. It has been made explicit.
There is an off by one error. The dust limit comparison uses < and therefore + 1 was not needed. It has been fixed.
It was not returning 0 amount for OP_RETURN outputs.

Script::dust_value has 2 problems.

The dust amount depends on minrelaytxfee which is configurable in Bitcoin Core. This method was not configurable.
The division operation was done before multiplying the byte amount, which can cause small differences when using uncommon scripts and minrelaytxfee values.

apoelstra · 2023-12-06T22:26:24Z

concept ACK. I am ambivalent about taking the minrelayfee as an argument. Maybe we should promote it to a constant, take it as an arg to minimal_non_dust, and show example code that passes the constant as the arg?

yancyribbens · 2023-12-06T22:27:23Z

A test or two would be nice :P

apoelstra

ACK b4f1bbb

tcharding · 2023-12-06T23:05:50Z

Perhaps this is cleaner?

    pub fn minimal_non_dust(script_pubkey: ScriptBuf) -> Self {
        // This is the default -minrelaytxfee (0.00001 BTC/kvB) in satoshis.
        const MIN_RELAY_TX_FEE: u64 = 1000; // sat/kvB
        const DUST_FEE_RATE: u64 3;

        let len = size_from_script_pubkey(&script_pubkey);
        let len = len
            + if script_pubkey.is_witness_program() {
                32 + 4 + 1 + (107 / 4) + 4
            } else {
                32 + 4 + 1 + 107 + 4
            };

        let limit = (len as u64) * DUST_FEE_RATE * (MIN_RELAY_TX_FEE / 1000);

        TxOut {
            value: Amount::from_sat(limit),
            script_pubkey,
        }
    }

IMO no tests needed for this code change.

yancyribbens · 2023-12-06T23:44:03Z

IMO no tests needed for this code change.

yeah no tests to begin with so.

junderw · 2023-12-07T00:02:21Z

Discovery: We had duplicate code in TxOut and Script.

I consolidated it to Script and changed stuff around.

junderw · 2023-12-07T00:04:17Z

The commit message needs changing heh.

Kixunil · 2023-12-07T00:38:26Z

I'm not convinced we should take the parameter since it's hard-coded in Core. As I understand it if Core changes it a lot of applications break so it won't happen.

Also if we're going to add it I'd prefer two method, the second one with _custom suffix instead of Option.

junderw · 2023-12-07T00:56:10Z

I'm not convinced we should take the parameter since it's hard-coded in Core. As I understand it if Core changes it a lot of applications break so it won't happen.

Also if we're going to add it I'd prefer two method, the second one with _custom suffix instead of Option.

The -minrelaytxfee arg for bitcoind will change the value for your node.

If we assume no one changes it, sure, a constant is fine. But Bitcoin Core allows users to change it on startup, so I think we should too.

junderw · 2023-12-07T01:42:48Z

I split things up into a _custom and normal function.

Kixunil · 2023-12-07T09:51:08Z

~~-minrelaytxfee is not related to the dust value. It's the total fee rate of a transaction. Dust value only applies to the outputs and as you can see the parameter is not used in Core.~~

So turns out I either misremembered it or the code changed since when I first read it. This is a dangerous situation - some contracts rely on the value being something specific. E.g. if enough people raised the value LN anchors would break. I don't think it should be easily configurable in Core (but of course we have to allow it in our code).

apoelstra · 2023-12-07T13:29:51Z

So turns out I either misremembered it or the code changed since when I first read it. This is a dangerous situation - some contracts rely on the value being something specific. E.g. if enough people raised the value LN anchors would break. I don't think it should be easily configurable in Core (but of course we have to allow it in our code).

If this is true then LN anchors are just broken. The minrelayfee effectively is increased whenever Core nodes' mempools exceed 300Kb (with default settings).

In any case, minrelayfee is purely a policy thing set by individual nodes. It absolutely should be configurable (it is impossible for the network to enforce any relay policy at all, including policy about minimum fees, so it is always "configurable" for somebody willing to do the work).

Whether we want to support different values, for purposes of defining a "dust limit" (another Core policy which nodes are free to implement differently), I don't know. These days it's usually a true assumption that nodes will enforce a dust limit at the default Core limit. In future, when we have consistent mempool backlog exceeding the default mempool size, it usually won't be. In future, plausibly Core will boost the value. Or drop it entirely and depend on the mempool eviction logic to drop low-fee transactions. Or something else entirely.

junderw · 2023-12-07T16:43:22Z

If this is true then LN anchors are just broken. The minrelayfee effectively is increased whenever Core nodes' mempools exceed 300Kb (with default settings).

It should be noted that the dust limit is not tied to the ejection rate (which is dynamic based on the mempool size) but just the runtime parameter.

apoelstra · 2023-12-07T16:49:51Z

@junderw oops, you're right. So txes above the minrelayfee will be relayed, they just might be immediately forgotten. This may be an important distinction.

I'm unsure how CPFP interacts with this. If it's possible to CPFP a too-low-fee transaction but not possible to CPFP a below-minrelayfee transaction, then that's a serious difference.

(All assuming that the vast majority of the network is using the default Core policy ofc)

junderw · 2023-12-07T17:27:47Z

Upon looking into the Core code to gather links to the areas where values are parsed and used... I noticed an error on my part.

I made a mistake: it was -dustrelayfee not -minrelaytxfee... but it is still configurable on startup.

(Initializing -dustrelayfee on startup)
https://github.com/bitcoin/bitcoin/blob/v26.0/src/init.cpp#L592
- (The default value is here, 3000)
  https://github.com/bitcoin/bitcoin/blob/v26.0/src/policy/policy.h#L55
(Initializing CTxMemPool options during startup in ApplyArgsManOptions)
https://github.com/bitcoin/bitcoin/blob/v26.0/src/init.cpp#L1468-L1475
- (Defining mempool_opts.dust_relay_feerate using the args passed in from init)
  https://github.com/bitcoin/bitcoin/blob/v26.0/src/node/mempool_args.cpp#L71-L79
(Adding CTxMemPool options to NodeContext)
https://github.com/bitcoin/bitcoin/blob/v26.0/src/init.cpp#L1485
(Defining m_dust_relay_feerate based on the opts)
https://github.com/bitcoin/bitcoin/blob/v26.0/src/txmempool.cpp#L410
(Using m_dust_relay_feerate for IsStandardTx)
https://github.com/bitcoin/bitcoin/blob/v26.0/src/validation.cpp#L721
(Using the dust_relay_fee in IsDust)
https://github.com/bitcoin/bitcoin/blob/v26.0/src/policy/policy.cpp#L144
(Using the dust_relay_fee in GetDustThreshold)
https://github.com/bitcoin/bitcoin/blob/v26.0/src/policy/policy.cpp#L67

sanket1729

ACK 09664c0

Kixunil

Not blocking but I would prefer my comments to be addressed.

bitcoin/src/blockdata/script/borrowed.rs

TxOut::minimal_non_dust has 3 problems. 1. There is an invisible dependency on Bitcoin Core's default minrelaytxfee value. It has been made explicit. 2. There is an off by one error. The dust limit comparison uses < and therefore `+ 1` was not needed. It has been fixed. 3. It was not returning 0 amount for OP_RETURN outputs. Script::dust_value has 2 problems. 1. The dust amount depends on minrelaytxfee which is configurable in Bitcoin Core. This method was not configurable. 2. The division operation was done before multiplying the byte amount, which can cause small differences when using uncommon scripts and minrelaytxfee values.

Kixunil

ACK 1b23220

bitcoin/src/blockdata/script/borrowed.rs

apoelstra

ACK 1b23220

junderw · 2023-12-10T22:16:42Z

Aaaahhh! Just realized the commit message still mistakenly refers to minrelaytxfee...... (On Mobile RN)

Kixunil · 2023-12-11T11:18:29Z

I can tolerate sligthly wrong commit message here.

tcharding · 2024-03-22T03:31:34Z

Was there a reason we didn't deprecate dust_value?

This change is hinders the upgrade path for upcoming 0.32.0 release.

apoelstra · 2024-03-22T13:20:33Z

I think we just forgot. Should we put it back before release?

c17db32 Pub back in deprecated dust_value (Tobin C. Harding) Pull request description: When we renamed `dust_value` to `minimal_non_dust` we forgot to keep the original and deprecated it, doing so assists with the upgrade path. Put back in deprecated `dust_value`, linking to the rename. Renamed in #2255, found while testing upgrade of downstream software. ACKs for top commit: tcharding: > ACK [c17db32](c17db32) I _think_ this matches the behavior of the old version apoelstra: ACK c17db32 I *think* this matches the behavior of the old version sanket1729: ACK c17db32 Tree-SHA512: 28e1bd2e1a0fd13c78c70ad2667b72b3bf649c293201b79c86c00f09d0126389ebaeb430b8dd32aeeec3d60cbd8761ae949f5784a5ea7756b1b9ae77ec96ce61

apoelstra previously approved these changes Dec 6, 2023

View reviewed changes

junderw dismissed apoelstra’s stale review via c2224e9 December 7, 2023 00:01

junderw force-pushed the patch-1 branch from b4f1bbb to c2224e9 Compare December 7, 2023 00:01

junderw force-pushed the patch-1 branch from c2224e9 to a1a8d8a Compare December 7, 2023 00:09

junderw changed the title ~~Fix: TxOut::minimal_non_dust~~ Fix: TxOut::minimal_non_dust and Script::dust_value Dec 7, 2023

junderw force-pushed the patch-1 branch from a1a8d8a to 3f74f13 Compare December 7, 2023 01:42

junderw force-pushed the patch-1 branch from 3f74f13 to 9ccf940 Compare December 7, 2023 17:26

junderw force-pushed the patch-1 branch from 9ccf940 to 09664c0 Compare December 7, 2023 17:28

sanket1729 previously approved these changes Dec 7, 2023

View reviewed changes

Kixunil reviewed Dec 7, 2023

View reviewed changes

junderw dismissed sanket1729’s stale review via 1b23220 December 8, 2023 05:55

junderw force-pushed the patch-1 branch from 09664c0 to 1b23220 Compare December 8, 2023 05:55

Kixunil approved these changes Dec 8, 2023

View reviewed changes

tcharding reviewed Dec 8, 2023

View reviewed changes

bitcoin/src/blockdata/script/borrowed.rs Show resolved Hide resolved

apoelstra approved these changes Dec 10, 2023

View reviewed changes

apoelstra merged commit c534027 into rust-bitcoin:master Dec 11, 2023
29 checks passed

tcharding mentioned this pull request Mar 22, 2024

Put back in deprecated dust_value #2625

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: TxOut::minimal_non_dust and Script::dust_value #2255

Fix: TxOut::minimal_non_dust and Script::dust_value #2255

junderw commented Dec 6, 2023 •

edited

Loading

apoelstra commented Dec 6, 2023

yancyribbens commented Dec 6, 2023

apoelstra left a comment

tcharding commented Dec 6, 2023

yancyribbens commented Dec 6, 2023

junderw commented Dec 7, 2023

junderw commented Dec 7, 2023

Kixunil commented Dec 7, 2023

junderw commented Dec 7, 2023

junderw commented Dec 7, 2023

Kixunil commented Dec 7, 2023 •

edited

Loading

apoelstra commented Dec 7, 2023

junderw commented Dec 7, 2023

apoelstra commented Dec 7, 2023

junderw commented Dec 7, 2023

sanket1729 left a comment

Kixunil left a comment

Kixunil left a comment

apoelstra left a comment

junderw commented Dec 10, 2023

Kixunil commented Dec 11, 2023

tcharding commented Mar 22, 2024

apoelstra commented Mar 22, 2024

Fix: TxOut::minimal_non_dust and Script::dust_value #2255

Fix: TxOut::minimal_non_dust and Script::dust_value #2255

Conversation

junderw commented Dec 6, 2023 • edited Loading

apoelstra commented Dec 6, 2023

yancyribbens commented Dec 6, 2023

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Dec 6, 2023

yancyribbens commented Dec 6, 2023

junderw commented Dec 7, 2023

junderw commented Dec 7, 2023

Kixunil commented Dec 7, 2023

junderw commented Dec 7, 2023

junderw commented Dec 7, 2023

Kixunil commented Dec 7, 2023 • edited Loading

apoelstra commented Dec 7, 2023

junderw commented Dec 7, 2023

apoelstra commented Dec 7, 2023

junderw commented Dec 7, 2023

sanket1729 left a comment

Choose a reason for hiding this comment

Kixunil left a comment

Choose a reason for hiding this comment

Kixunil left a comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

junderw commented Dec 10, 2023

Kixunil commented Dec 11, 2023

tcharding commented Mar 22, 2024

apoelstra commented Mar 22, 2024

junderw commented Dec 6, 2023 •

edited

Loading

Kixunil commented Dec 7, 2023 •

edited

Loading