Introduce some policy constants from Bitcoin Core #584

darosior · 2021-04-05T13:00:10Z

~~Not sure about the place since all other constants are consensus, but it seemed awkward to have it eg in the transaction module while there is a constants one 🤷~~

EDIT: this now introduces a minimal set of policy stuff from bitcoind in a new policy module.

stevenroose · 2021-04-05T20:41:22Z

Hmm I'm not sure how I feel about standardness constants in here because they depend on Bitcoin Core versions, so they might get outdated. Perhaps we can have a mod standard with such a disclaimer placed on it or so?

TheBlueMatt · 2021-04-05T21:09:35Z

Hmm I'm not sure how I feel about standardness constants in here because they depend on Bitcoin Core versions, so they might get outdated

There are effectively multiple different types of standardness rules - there are those that depend on Bitcoin Core versions and are likely to be relaxed as a part of a future soft-fork, and those which are effectively anti-DoS rules and are unlikely to be made more strict (though may be relaxed with algorithmic improvements). For better or for worse, things like the max standard weight and dust limits are now a part of the lightning trust model, which secures a nontrivial amount of funds. Changing them to be more strict in the future would imply a potential funds loss vulnerability in lightning.

dr-orlovsky

Probably outside of the scope of this PR we should try to add some consistentcy into which constants from Bitcoin Core we are moving to rust bitcoin and where they should reside.

For now, I have nothing against adding a single constant (it is better than none), but at least from here https://github.com/bitcoin/bitcoin/blob/4a540683ec40393d6369da1a9e02e45614db936d/src/policy/policy.h#L19-L54 I see a set of constants which can be added alongside MAX_STANDARD_TX_WEIGHT:

DEFAULT_BLOCK_MAX_WEIGHT
DEFAULT_BLOCK_MIN_TX_FEE
MIN_STANDARD_TX_NONWITNESS_SIZE

src/blockdata/constants.rs

darosior · 2021-04-23T12:50:54Z

I agree, i'll spot the constants i find appropriate and will include them here. Would you conceptually accept this @apoelstra ?

sgeisler · 2021-04-23T17:16:37Z

For the record, I like the idea of having a separate module for standardness constants with a disclaimer as proposed by @stevenroose. Iiuc that would also be a good place for the constants introduced in #566 @TheBlueMatt?

apoelstra · 2021-04-27T21:48:24Z

concept ACK having this value in here. I agree that "in theory" it could change but in practice nobody has ever proposed to change it, and in future Taproot will make massive transactions even less useful.

darosior · 2021-05-12T20:16:12Z

Added some constants that i found sensible, leaving the Script-related ones in rust-miniscipt, though we might want to include them here as well if non-Miniscript users want to access them too? rust-miniscript could access them from here once/if we have a policy module. Happy to introduce more if i missed some!

Renamed MAX_STANDARD_TRANSACTION_WEIGHT to MAX_STANDARD_TX_WEIGHT and added a counterpart to bitcoind's GetVirtualTransactionSize, which is now possible since this introduces the sigop cost constant.

Overall i tried to use the same units as bitcoind, so it results in a lot of coercion (but only on constants!).

For now, I have nothing against adding a single constant (it is better than none), but at least from here https://github.com/bitcoin/bitcoin/blob/4a540683ec40393d6369da1a9e02e45614db936d/src/policy/policy.h#L19-L54 I see a set of constants which can be added alongside MAX_STANDARD_TX_WEIGHT:

DEFAULT_BLOCK_MAX_WEIGHT

DEFAULT_BLOCK_MIN_TX_FEE

MIN_STANDARD_TX_NONWITNESS_SIZE

I don't think that it's sensible to add DEFAULT_BLOCK_MAX_WEIGHT and DEFAULT_BLOCK_MIN_TX_FEE. It's internal to Bitcoin Core, only used in the mining code (and we don't have any here?). Added MIN_STANDARD_TX_NONWITNESS_SIZE.

dr-orlovsky

Checked against constant values in Bitcoin Core. General utACK with few nits.

src/policy/mod.rs

src/lib.rs

sgeisler

utACK 05a813a

Left some minor nits and questions. But nothing blocking if you want to go ahead and merge.

src/blockdata/constants.rs

src/lib.rs

src/policy/mod.rs

This introduces some constants defined by Bitcoin Core which as a consequence define some network rules in a new 'policy' module. Only some were picked, which are very unlikely to change. Nonetheless a Warning has been put in the module documentation. Script-level constants are left into rust-miniscript where they are already defined (src/miniscript/limits.rs).

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

It's very useful to Bitcoin applications, and especially "L2" ones, to effectively compute feerates. Currently (and this is very unlikely to change) bitcoind nodes compute the virtual size as a rounded-up division of the size in witness units by 4, with a penalty for transactions that are essentially >5% full of sigops. Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

darosior · 2021-05-18T13:57:58Z

Added two more constants that i forgot and are useful to contract and wallet developers: the default minimum relay feerate and the default mempool expiration.

sgeisler

tACK 7345aa6

Test log:

May 21 01:13:36.988  INFO testinator: Installing rust toolchain 'nightly'
May 21 01:14:05.342  INFO testinator: Installing rust toolchain 'stable'
May 21 01:15:31.062  INFO testinator: Installing rust toolchain '1.29.0'
May 21 01:15:32.248  INFO testinator: Preparing environment for rust 1.29.0 tests (8 configurations)
May 21 01:15:32.249  INFO testinator: Preparing environment for rust stable tests (8 configurations)
May 21 01:15:32.249  INFO testinator: Preparing environment for rust nightly tests (9 configurations)
May 21 01:19:55.811  INFO testinator: Running rust 1.29.0 tests in /tmp/rust-bitcoin-1.29.0.nsz3Ox9Bjc22/rust-bitcoin
May 21 01:19:55.812 DEBUG testinator: Generating lock file with rust=1.29.0
May 21 01:19:55.816  INFO testinator: Running rust nightly tests in /tmp/rust-bitcoin-nightly.3qsFJJiFwmF2/rust-bitcoin
May 21 01:19:55.819  INFO testinator: Running rust stable tests in /tmp/rust-bitcoin-stable.aSpzKLNvwhxi/rust-bitcoin
May 21 01:20:12.659 DEBUG testinator: Pinning cc to 1.0.41
May 21 01:20:12.915 DEBUG testinator: Pinning serde to 1.0.98
May 21 01:20:14.681 DEBUG testinator: Pinning serde_derive to 1.0.98
May 21 01:20:15.010 DEBUG testinator: Pinning byteorder to 1.3.4
May 21 01:21:14.712  INFO testinator: Test rust=nightly, features=[secp-recovery] succeeded!
May 21 01:21:16.782  INFO testinator: Test rust=stable, features=[secp-recovery] succeeded!
May 21 01:22:11.653  INFO testinator: Test rust=stable, features=[use-serde] succeeded!
May 21 01:22:17.784  INFO testinator: Test rust=nightly, features=[use-serde] succeeded!
May 21 01:22:18.260  INFO testinator: Test rust=1.29.0, features=[secp-recovery] succeeded!
May 21 01:22:41.497  INFO testinator: Test rust=nightly, features=[base64] succeeded!
May 21 01:22:42.098  INFO testinator: Test rust=stable, features=[base64] succeeded!
May 21 01:23:05.622  INFO testinator: Test rust=nightly, features=[secp-lowmemory] succeeded!
May 21 01:23:07.126  INFO testinator: Test rust=stable, features=[secp-lowmemory] succeeded!
May 21 01:23:28.823  INFO testinator: Test rust=nightly, features=[rand] succeeded!
May 21 01:23:31.392  INFO testinator: Test rust=stable, features=[rand] succeeded!
May 21 01:23:52.468  INFO testinator: Test rust=nightly, features=[unstable] succeeded!
May 21 01:24:12.885  INFO testinator: Test rust=stable, features=[bitcoinconsensus] succeeded!
May 21 01:24:14.254  INFO testinator: Test rust=1.29.0, features=[use-serde] succeeded!
May 21 01:24:43.747  INFO testinator: Test rust=stable, features=[secp-recovery,use-serde,base64,secp-lowmemory,rand,bitcoinconsensus] succeeded!
May 21 01:24:55.391  INFO testinator: Test rust=nightly, features=[bitcoinconsensus] succeeded!
May 21 01:25:19.925  INFO testinator: Test rust=stable, features=[] succeeded!
May 21 01:25:21.996  INFO testinator: Test rust=1.29.0, features=[base64] succeeded!
May 21 01:25:30.077  INFO testinator: Test rust=nightly, features=[secp-recovery,use-serde,base64,secp-lowmemory,rand,unstable,bitcoinconsensus] succeeded!
May 21 01:26:01.512  INFO testinator: Test rust=nightly, features=[] succeeded!
May 21 01:26:12.570  INFO testinator: Test rust=1.29.0, features=[secp-lowmemory] succeeded!
May 21 01:26:57.964  INFO testinator: Test rust=1.29.0, features=[rand] succeeded!
May 21 01:27:45.987  INFO testinator: Test rust=1.29.0, features=[bitcoinconsensus] succeeded!
May 21 01:28:38.155  INFO testinator: Test rust=1.29.0, features=[secp-recovery,use-serde,base64,secp-lowmemory,rand,bitcoinconsensus] succeeded!
May 21 01:29:25.015  INFO testinator: Test rust=1.29.0, features=[] succeeded!
May 21 01:29:27.461  INFO testinator: Fuzzing deserialize_script
May 21 01:30:43.060  INFO testinator: Successfully fuzzed deserialize_script
May 21 01:30:43.060  INFO testinator: Fuzzing uint128_fuzz
May 21 01:31:44.154  INFO testinator: Successfully fuzzed uint128_fuzz
May 21 01:31:44.154  INFO testinator: Fuzzing deserialize_amount
May 21 01:32:45.117  INFO testinator: Successfully fuzzed deserialize_amount
May 21 01:32:45.117  INFO testinator: Fuzzing deserialize_transaction
May 21 01:33:47.297  INFO testinator: Successfully fuzzed deserialize_transaction
May 21 01:33:47.297  INFO testinator: Fuzzing deser_net_msg
May 21 01:34:50.147  INFO testinator: Successfully fuzzed deser_net_msg
May 21 01:34:50.147  INFO testinator: Fuzzing deserialize_address
May 21 01:35:51.105  INFO testinator: Successfully fuzzed deserialize_address
May 21 01:35:51.105  INFO testinator: Fuzzing deserialize_block
May 21 01:36:53.206  INFO testinator: Successfully fuzzed deserialize_block
May 21 01:36:53.206  INFO testinator: Fuzzing outpoint_string
May 21 01:37:55.110  INFO testinator: Successfully fuzzed outpoint_string
May 21 01:37:55.110  INFO testinator: Fuzzing deserialize_psbt
May 21 01:38:58.117  INFO testinator: Successfully fuzzed deserialize_psbt

dr-orlovsky

ACK, re-checked against Bitcoin Core values. Two nits for comments which can be ignored. If not, pls add them as a separate commit so there will be no need to re-check constant values against Bitcoin Core

dr-orlovsky · 2021-05-23T13:14:57Z

src/policy/mod.rs

+//! Policy
+//!
+//! This module exposes some constants and functions used in the reference implementation and which
+//! as a consequence defines some network rules.


NIT typo

Suggested change

//! as a consequence defines some network rules.

//! as a consequence define some network rules.

dr-orlovsky · 2021-05-23T13:21:27Z

src/policy/mod.rs

+/// mempools.
+pub const DEFAULT_MEMPOOL_EXPIRY: u32 = 336;
+
+/// The virtual transaction size, as computed by default by bitcoind node.


Nit (this is more clear description):

Suggested change

/// The virtual transaction size, as computed by default by bitcoind node.

/// The virtual transaction size, as computed by Bitcoin Core `GetVirtualTransactionSize` using default bytes per sigop value

stevenroose

utACK

sgeisler · 2021-05-29T00:50:15Z

3 ACKs, CI+1 local test run, minor API change: merged

darosior force-pushed the max_tx_weight_const branch from dd9e837 to 4a3c771 Compare April 5, 2021 13:04

dr-orlovsky suggested changes Apr 6, 2021

View reviewed changes

src/blockdata/constants.rs Outdated Show resolved Hide resolved

dr-orlovsky mentioned this pull request Apr 6, 2021

Fix Script::dust_value()'s calculation for non-P2*PKH script_pubkeys #579

Merged

darosior force-pushed the max_tx_weight_const branch from 4a3c771 to 9995ce3 Compare May 12, 2021 20:16

darosior requested a review from dr-orlovsky May 12, 2021 20:16

darosior changed the title ~~blockdata: add the maximum standard transaction weight~~ Introduce some policy constants from Bitcoin Core May 12, 2021

darosior force-pushed the max_tx_weight_const branch from 9995ce3 to 05a813a Compare May 12, 2021 20:18

dr-orlovsky previously approved these changes May 13, 2021

View reviewed changes

src/policy/mod.rs Show resolved Hide resolved

src/lib.rs Show resolved Hide resolved

sgeisler previously approved these changes May 13, 2021

View reviewed changes

src/blockdata/constants.rs Outdated Show resolved Hide resolved

src/lib.rs Show resolved Hide resolved

src/policy/mod.rs Show resolved Hide resolved

src/policy/mod.rs Show resolved Hide resolved

darosior dismissed stale reviews from sgeisler and dr-orlovsky via 7d91d9c May 13, 2021 15:31

darosior force-pushed the max_tx_weight_const branch from 05a813a to 7d91d9c Compare May 13, 2021 15:31

darosior added 3 commits May 18, 2021 14:52

blockdata/script: use policy's constant in dust computation

2e9d62a

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

darosior force-pushed the max_tx_weight_const branch from 7d91d9c to 7345aa6 Compare May 18, 2021 13:25

sgeisler approved these changes May 20, 2021

View reviewed changes

dr-orlovsky approved these changes May 23, 2021

View reviewed changes

sgeisler added the minor API Change This PR should get a minor version bump label May 23, 2021

dr-orlovsky added this to the 0.26.1 milestone May 24, 2021

stevenroose approved these changes May 24, 2021

View reviewed changes

sgeisler merged commit d7eb15c into rust-bitcoin:master May 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce some policy constants from Bitcoin Core #584

Introduce some policy constants from Bitcoin Core #584

darosior commented Apr 5, 2021 •

edited

stevenroose commented Apr 5, 2021

TheBlueMatt commented Apr 5, 2021

dr-orlovsky left a comment

darosior commented Apr 23, 2021

sgeisler commented Apr 23, 2021

apoelstra commented Apr 27, 2021

darosior commented May 12, 2021

dr-orlovsky left a comment

sgeisler left a comment

darosior commented May 18, 2021

sgeisler left a comment •

edited

dr-orlovsky left a comment

dr-orlovsky May 23, 2021

dr-orlovsky May 23, 2021

stevenroose left a comment

sgeisler commented May 29, 2021

	//! as a consequence defines some network rules.
	//! as a consequence define some network rules.

	/// The virtual transaction size, as computed by default by bitcoind node.
	/// The virtual transaction size, as computed by Bitcoin Core `GetVirtualTransactionSize` using default bytes per sigop value

Introduce some policy constants from Bitcoin Core #584

Introduce some policy constants from Bitcoin Core #584

Conversation

darosior commented Apr 5, 2021 • edited

stevenroose commented Apr 5, 2021

TheBlueMatt commented Apr 5, 2021

dr-orlovsky left a comment

Choose a reason for hiding this comment

darosior commented Apr 23, 2021

sgeisler commented Apr 23, 2021

apoelstra commented Apr 27, 2021

darosior commented May 12, 2021

dr-orlovsky left a comment

Choose a reason for hiding this comment

sgeisler left a comment

Choose a reason for hiding this comment

darosior commented May 18, 2021

sgeisler left a comment • edited

Choose a reason for hiding this comment

dr-orlovsky left a comment

Choose a reason for hiding this comment

dr-orlovsky May 23, 2021

Choose a reason for hiding this comment

dr-orlovsky May 23, 2021

Choose a reason for hiding this comment

stevenroose left a comment

Choose a reason for hiding this comment

sgeisler commented May 29, 2021

darosior commented Apr 5, 2021 •

edited

sgeisler left a comment •

edited