Export standard Script flags in bitcoinconsensus

ariard commented at 8:11 am on April 28, 2020: member

Bitcoinconsensus library makes available consensus script verification flags to other applications. Currently this doesn’t enable to verify transaction scripts correctness with regards to deployed standard rules. A defect of adherance to this set of rules hinders severely transaction propagation on the network, and therefore may be a concern if application has a time-sensitive requirement (LN, Atomic Swaps, …).

Should API version be bumped ?
Should documentation/flags name clearly sort between standard/consensus rules ? (while minding that standard/consensus enforcement may vary between witness versions, cf MINIMALIF)
Should some obscure standard flags (CONST_SCRIPTCODE) gets a real specification ?

See https://github.com/lightningnetwork/lightning-rfc/pull/764 as example of protocol specification directly requiring adherence to standard rules.

sipa commented at 8:20 am on April 28, 2020: member

I think it may make sense to export some notion of standardness from libconsensus, but I don’t think exporting every individual script flag of the interpreter makes sense.

Would it be sufficient for your use case if there was just a catch-all libconsensus_SCRIPT_VERIFY_STANDARD (which would always refer to whatever the current version’s policy was, with no guarantees for consistency between versions).

ariard commented at 8:40 am on April 28, 2020: member

@sipa note I don’t target specifically one use case even if issue has been raised in LN context. It should be a good practice to verify any transaction against current version’s policy, even for application with lower requirement, to avoid any surprise for user.

SCRIPT_VERIFY_STANDARD would be fine and that was my intention by defining a SCRIPT_FLAGS_VERIFY_STANDARD. But I’m not sure if such global opaque flag would work everytime as some rule may be either standard or consensus following transaction/witness version. Like MINIMALIF between segwit v0 and v1 and therefore you may need access for flag granularity ?

You do want to avoid some application evaluating transaction as non-standard when it’s in fact invalid, even when it’s malleable and can be corrected like MINIMALIF (“sending directly your taproot spending to a miner and not being able to respond to its rejection”)

DrahtBot added the label Consensus on Apr 28, 2020

DrahtBot added the label Docs on Apr 28, 2020

sipa commented at 9:30 pm on April 28, 2020: member

@ariard I guess a hypothetical SCRIPT_VERIFY_STANDARD flag would automatically also enable all consensus rules. If you care about whether your transaction will be acceptable to full nodes similar to the one whose libconsensus you’re using, it doesn’t make sense to disable certain consensus checks but still enable standardness ones.

I don’t understand what you’re trying to distinguish in your second comment.

in src/script/bitcoinconsensus.h:58 in e07ecdedc7 outdated

74+    bitcoinconsensus_SCRIPT_FLAGS_VERIFY_DISCOURAGE_UPGRADABLE_WITNESS_PROGRAM = (1U << 12), // discourage use of v1-v16 witness program
75+    bitcoinconsensus_SCRIPT_FLAGS_VERIFY_MINIMALIF                             = (1U << 13), // enforce MINIMALIF (BIP342)
76+    bitcoinconsensus_SCRIPT_FLAGS_VERIFY_NULLFAIL                              = (1U << 14), // enforce NULLFAIL (BIP146)
77+    bitcoinconsensus_SCRIPT_FLAGS_VERIFY_WITNESS_PUBKEYTYPE                    = (1U << 15), // enforce compression of pubkey in segwit scripts
78+    bitcoinconsensus_SCRIPT_FLAGS_VERIFY_CONST_SCRIPTCODE                      = (1U << 16), // enforce constant scriptCode policy in non-segwit scripts
79+    bitcoinconsensus_SCRIPT_FLAGS_VERIFY_ALL                                   = bitcoinconsensus_SCRIPT_FLAGS_VERIFY_P2SH | bitcoinconsensus_SCRIPT_FLAGS_VERIFY_DERSIG |

ryanofsky commented at 10:02 pm on April 28, 2020:

In commit “[policy] Extend bitcoinconsensus library with standard script flags” (e07ecdedc70ea045f98b592d36fdbf543696db28)

It seems like the name VERIFY_ALL is going to be misleading if it isn’t actually going to include every verify flag. Would maybe suggest calling it VERIFY_CONSENSUS and changing VERIFY_ALL into a deprecated alias for VERIFY_CONSENSUS

ariard commented at 6:09 am on April 29, 2020:

I would like but this may break upstream libraries using exported flag? So need an API bump, we can do this.

ryanofsky commented at 11:21 am on April 29, 2020:

Not suggesting API bump. Suggestion is to use VERIFY_CONSENSUS instead of VERIFY_ALL in main definition for clarity in new code and add VERIFY_ALL = VERIFY_CONSENSUS /* deprecated */ at the bottom for backwards compatibility with old code

ryanofsky commented at 10:25 pm on April 28, 2020: member

Concept ACK for bed1e9f6ab525441c432ed0722fe76101b3f896f. I didn’t closely check the code since it sounds it might be pared down to just add a VERIFY_STANDARD flag. But both current and suggest approach seem reasonable to me.

I don’t understand what you’re trying to distinguish in your second comment.

I could be misinterpreting the comment, but I think it’s saying a drawback of only adding a SCRIPT_VERIFY_STANDARD flag is that the API wouldn’t be giving applications a very convenient way of distinguishing between invalid and nonstandard transactions. They might have to call the verify function twice. Perhaps a new bitcoinconsensus_error_t value could help with this, though. @sipa you might also want to say if you have a preference for where to implement VERIFY_STANDARD (script/script or script/bitcoinconsensus) in case there are concerns like keeping the flag up to date with future standardness changes

ariard commented at 6:07 am on April 29, 2020: member

it doesn’t make sense to disable certain consensus checks but still enable standardness ones.

Yes right, you would need to call verify function twice to dissociate between standard/consensus in case of failure but that’s acceptable. Let’s do a global standard flag.

in case there are concerns like keeping the flag up to date with future standardness changes

Yes it should be easy to keep in check exported standard flags with regards to future changes, we can make available STANDARD_SCRIPT_VERIFY_FLAGS from policy.h?

luke-jr commented at 5:56 pm on April 30, 2020: member

Concept NACK.

Node policy is a per-node decision. It should not be assumed to have any global consistency, and users should always be able to easily modify it (libconsensus should definitely not read bitcoin.conf!).

Furthermore, libconsensus is for consensus logic. This is strictly not that.

sipa commented at 6:04 pm on April 30, 2020: member

I strongly feel we should not be exporting all the nitty details of how Bitcoin Core implements standardness. There is no requirement that the various standardness VERIFY flags have any consistency across versions.

Either you ask for consensus-validity, and specify which consensus rules you want to enable; or you ask for standardness without the ability to select what parts of it.

luke-jr commented at 6:57 pm on April 30, 2020: member

To help provide clarity: libconsensus is supposed to be just raw Bitcoin, not Bitcoin Core (implementation details aside).

TheBlueMatt commented at 8:13 pm on April 30, 2020: member

I tend to agree with @sipa here, we definitely shouldn’t be trying to export all of the internal flags in Bitcoin Core as a reasonably-supported interface, but at the same time, not exporting some basic concept of “will this transaction likely be accepted by the network” limits the utility of libbitcoinconsensus as it exists today to…very little. Certainly it wasn’t the initial goal of libbitcoinconsensus, and makes the name somewhat of a misnomer, but it makes libbitcoinconsensus a pretty useful primitive for checking a standalone transaction.

MarcoFalke commented at 8:24 pm on April 30, 2020: member

Concept ACK to expose this as a cheap sanity pre-check for testmempoolaccept, but for a full policy check you’ll have to ask the mempool of a full node

ariard force-pushed on May 3, 2020

ariard commented at 9:11 am on May 3, 2020: member

Updated with only exporting a simple verify_script_standard_with_amount method, without flags passing, therefore making standard opaque to caller.

I’m making the point again that this export isn’t only useful for testing cases but also for production. Time-sensitive protocols like LN or Coinswap rely on confirming onchain transaction before some time-lock expiration. Doing so means they need to guarantee that these transactions broadcast smoothly on the currently deployed network, therefore have a higher concern with regards to policy. A flaw to adhere to standardness hinders their security.

It has been made the argument during the IRC meeting, that for these protocols, they have well-defined transactions format and also witnessScript, which is true. But standard rules don’t scope themselves only on this in scriptPubKey validation, they may cover witness element not part of the script and not known before production. A rule like SCRIPT_VERIFY_WITNESS_PUBKEYTYPE concerning pubkeys in witness v0 is such example.

These witness elements, unknown before protocol running, may be provided by a counterparty while a cooperative witness construction to allow a spend of a multisig output. If this spent is time-sensitive, and therefore its good propagation a requirement, this counterparty input must be sanitized. A counterparty may have interest in your transaction not confirming.

That’s where having a function like verify_script_standard is useful for applications. It’s a better remedy than developers directly copy-pasting standard flags (like https://github.com/ACINQ/bitcoin-lib/blob/master/src/main/scala/fr/acinq/bitcoin/Script.scala), and taking the risk of non-updating for conformity at policy upgrade.

A call to testmempoolaccept wouldn’t fit for different reasons, given that an offchain transaction you’re verifying may have an offchain parent too, and would be considered as an orphan by the mempool hindering other policy rule violation you’re interested too. Fees is another issue as a minimal transaction feerate may be pre-agreed between parties and planned to be unilaterally RBF/CPFP’ed at broadcast.

For these reasons, I think enabling standardness verification at the script level make sense, as its correctness, disentangled from more-dynamic policy rules like packages size or fees, already avoid some issues for this class of applications.

in src/script/bitcoinconsensus.h:51 in a7a0c4c761 outdated

56-    bitcoinconsensus_SCRIPT_FLAGS_VERIFY_CHECKSEQUENCEVERIFY = (1U << 10), // enable CHECKSEQUENCEVERIFY (BIP112)
57-    bitcoinconsensus_SCRIPT_FLAGS_VERIFY_WITNESS             = (1U << 11), // enable WITNESS (BIP141)
58-    bitcoinconsensus_SCRIPT_FLAGS_VERIFY_ALL                 = bitcoinconsensus_SCRIPT_FLAGS_VERIFY_P2SH | bitcoinconsensus_SCRIPT_FLAGS_VERIFY_DERSIG |
59-                                                               bitcoinconsensus_SCRIPT_FLAGS_VERIFY_NULLDUMMY | bitcoinconsensus_SCRIPT_FLAGS_VERIFY_CHECKLOCKTIMEVERIFY |
60-                                                               bitcoinconsensus_SCRIPT_FLAGS_VERIFY_CHECKSEQUENCEVERIFY | bitcoinconsensus_SCRIPT_FLAGS_VERIFY_WITNESS
61+    bitcoinconsensus_SCRIPT_FLAGS_VERIFY_NONE                                  = 0,

ryanofsky commented at 2:15 am on May 7, 2020:

In commit “[policy] Extend bitcoinconsensus library with standard script flags” (a7a0c4c7612efbf95849cd70c28497d27b3386c2)

Should drop the whitespace changes here. Assuming they are unintentional

ariard commented at 9:24 am on May 7, 2020:

Oh right, they weren’t disclosed by git show -b locally.

in src/script/bitcoinconsensus.h:77 in a7a0c4c761 outdated

71@@ -72,6 +72,14 @@ EXPORT_SYMBOL int bitcoinconsensus_verify_script_with_amount(const unsigned char
72                                     const unsigned char *txTo        , unsigned int txToLen,
73                                     unsigned int nIn, unsigned int flags, bitcoinconsensus_error* err);
74 
75+/// Returns 1 if the input nIn of the serialized transaction pointed to by
76+/// txTo correctly spends the scriptPubKey pointed to by scriptPubKey under
77+/// the current standardness constraints.

ryanofsky commented at 2:28 am on May 7, 2020:

In commit “[policy] Extend bitcoinconsensus library with standard script flags” (a7a0c4c7612efbf95849cd70c28497d27b3386c2)

Maybe extend comment to say something about limitations of this function, e.g. that it only reflects default policy in the current release, actual policy can vary between nodes, software versions

in doc/shared-libraries.md:8 in 4ea5e6d182 outdated

4@@ -5,6 +5,11 @@ Shared Libraries
5 
6 The purpose of this library is to make the verification functionality that is critical to Bitcoin's consensus available to other applications, e.g. to language bindings.
7 
8+Policy correctness as enforced by latest software version is also made available. To dissociate between invalidity reasons, either consensus or standard, an application

ryanofsky commented at 2:30 am on May 7, 2020:

In commit “[doc] Document shared-libraries.md on standard verification” (4ea5e6d18203f1c45fa68b8f5585c7fc96e5677b)

Maybe standardize on standardness: s/correctness/standardness/ and s/standard/standardness/

ryanofsky approved

ryanofsky commented at 2:34 am on May 7, 2020: member

Code review ACK 4ea5e6d18203f1c45fa68b8f5585c7fc96e5677b. Left minor suggestions, feel free to ignore

ariard force-pushed on May 7, 2020

ariard commented at 9:26 am on May 7, 2020: member

Thanks Russ for review, took your points, specially call-up on better documentation.

in src/script/bitcoinconsensus.h:59 in e68badd2cc outdated

55@@ -56,8 +56,8 @@ enum
56     bitcoinconsensus_SCRIPT_FLAGS_VERIFY_CHECKSEQUENCEVERIFY = (1U << 10), // enable CHECKSEQUENCEVERIFY (BIP112)
57     bitcoinconsensus_SCRIPT_FLAGS_VERIFY_WITNESS             = (1U << 11), // enable WITNESS (BIP141)
58     bitcoinconsensus_SCRIPT_FLAGS_VERIFY_ALL                 = bitcoinconsensus_SCRIPT_FLAGS_VERIFY_P2SH | bitcoinconsensus_SCRIPT_FLAGS_VERIFY_DERSIG |
59-                                                               bitcoinconsensus_SCRIPT_FLAGS_VERIFY_NULLDUMMY | bitcoinconsensus_SCRIPT_FLAGS_VERIFY_CHECKLOCKTIMEVERIFY |
60-                                                               bitcoinconsensus_SCRIPT_FLAGS_VERIFY_CHECKSEQUENCEVERIFY | bitcoinconsensus_SCRIPT_FLAGS_VERIFY_WITNESS
61+                                                             bitcoinconsensus_SCRIPT_FLAGS_VERIFY_NULLDUMMY | bitcoinconsensus_SCRIPT_FLAGS_VERIFY_CHECKLOCKTIMEVERIFY |

ryanofsky commented at 12:17 pm on May 7, 2020:

In commit “[policy] Extend bitcoinconsensus library with standard script flags” (e68badd2ccc2fbced8663127f2f3d94d8029e903)

There are still extra whitespace changes here that could be reverted

ryanofsky approved

ryanofsky commented at 12:18 pm on May 7, 2020: member

Code review ACK 4b21d9d7c2db1f50ca431650c14e8360873030cc. Just documentation updates and whitespace fixes since last review

[policy] Extend bitcoinconsensus library with standard script flags

Bitcoinconsensus library makes available consensus script verification
flags to other applications. Currently this doesn't enable to verify
transaction scripts correctness with regards to deployed standard rules.
A defect of adherance to this set of rules hinders severely transaction
propagation on the network, and therefore may be a concern if
application has a time-sensitive requirement (LN, Atomic Swaps, ...).

Export verify_script_standard_with_amount enforcing standard correctness
of a spent.

3a931ed82e

[doc] Document shared-libraries.md on standard verification

Add reference to rust-bitcoinconsensus.

9f0a6f36b5

ariard force-pushed on May 14, 2020

ryanofsky approved

ryanofsky commented at 9:52 pm on May 14, 2020: member

Code review ACK 9f0a6f36b5ccf08e054af63d242c8b98344676b0. Just reverted the last remaining flags change (whitespace) since the previous review.

The code change here it trivial. It needs more reviewers, but as far as I can tell all the concerns previously raised about this PR were addressed, so maybe this can move along

elichai commented at 5:57 pm on May 26, 2020: contributor

Code Review ACK 9f0a6f36b5ccf08e054af63d242c8b98344676b0 I do believe that even though standardness is an implementation detail in practice it’s important to follow if you want your transactions to actually make their way to miners through the p2p, so you’ll want to follow it even if you don’t use bitcoin core.

laanwj added this to the "Blockers" column in a project

in doc/shared-libraries.md:8 in 9f0a6f36b5

4@@ -5,6 +5,12 @@ Shared Libraries
5 
6 The purpose of this library is to make the verification functionality that is critical to Bitcoin's consensus available to other applications, e.g. to language bindings.
7 
8+Policy standardness as enforced by latest software version is also made available. To dissociate between invalidity reasons, either consensus or standardness, an

fjahr commented at 2:57 pm on June 16, 2020:

nit-ish: This whole block seems to be weirdly placed. Shouldn’t this be below Script validation? Or at least under API. It is documenting a new part of the API…

ariard commented at 4:44 pm on June 17, 2020:

API is really about where to find its definition in the codebase. New documentation is about explaining how to use API and its limitations, but I agree I should add bitcoinconsensus_verify_script_standard_with_amount in Script Validation. Will do if I have to rebase/modify PR.

laanwj commented at 1:50 pm on June 18, 2020:

Tend to agree here. This is an introductory paragraph and while you could add one sentence mentioning that there’s also a policy function, it shouldn’t describe the details. That can be done separately in a new section, for example.

fjahr approved

fjahr commented at 3:06 pm on June 16, 2020: member

Code review ACK 9f0a6f3

If you retouch I think the docs could be improved a bit more.

MarcoFalke removed the label Docs on Jun 17, 2020

ariard commented at 4:45 pm on June 17, 2020: member

@luke-jr If you still have concerns on this can you raise them before PR moves forward ? See #18797 (comment) as a motivation.

luke-jr commented at 5:59 pm on June 17, 2020: member

I’m making the point again that this export isn’t only useful for testing cases but also for production. Time-sensitive protocols like LN or Coinswap rely on confirming onchain transaction before some time-lock expiration. Doing so means they need to guarantee that these transactions broadcast smoothly on the currently deployed network, therefore have a higher concern with regards to policy. A flaw to adhere to standardness hinders their security.

Policy can never be guaranteed, and shouldn’t be trusted to be uniform. Acting on policy is almost(?) always a design flaw.

Core’s policies also do not define what is a standard (they tolerate various non-standard things). If you want to check what conforms to standards, you would need entirely new code, which makes more sense as an entirely separate library.

Protocols like Lightning need to, and AFAIK already do, define strict transaction templates in hopes they will be accepted by node policies.

It has been made the argument during the IRC meeting, that for these protocols, they have well-defined transactions format and also witnessScript, which is true. But standard rules don’t scope themselves only on this in scriptPubKey validation, they may cover witness element not part of the script and not known before production. A rule like SCRIPT_VERIFY_WITNESS_PUBKEYTYPE concerning pubkeys in witness v0 is such example.

Seems like more of an argument for a key validity check, which can optionally check expected future rules too (presumably overlapping with policy in such matters).

A call to testmempoolaccept wouldn’t fit for different reasons, given that an offchain transaction you’re verifying may have an offchain parent too, and would be considered as an orphan by the mempool hindering other policy rule violation you’re interested too.

Sounds like an argument for enabling a chain of txs in testmempoolaccept :)

Fees is another issue as a minimal transaction feerate may be pre-agreed between parties and planned to be unilaterally RBF/CPFP’ed at broadcast.

Test the highest-fee variant?

laanwj added the label Utils/log/libs on Jun 18, 2020

ariard commented at 8:37 pm on June 18, 2020: member

@luke-jr

Policy can never be guaranteed, and shouldn’t be trusted to be uniform. Acting on policy is almost(?) always a design flaw.

How can we be sure we’re not acting on policy if its scope is not well-defined ? Sounds like a) policy is a moving target but b) be sure you’re not making any assumption on a moving target :)

Core’s policies also do not define what is a standard (they tolerate various non-standard things). If you want to check what conforms to standards, you would need entirely new code, which makes more sense as an entirely separate library.

Again, how can you say they tolerate non-standard things if you don’t have a standard norm to refer to ? Can you point to those standards you’re mentioning ? How can we check conformity if it’s not defined by some spec or enforced by some code ?

Protocols like Lightning need to, and AFAIK already do, define strict transaction templates in hopes they will be accepted by node policies.

Ensuring propagation of time-sensitive transaction is critical for this kind of application. Even if Lightning BOLTs define strict transaction and script templates (and honestly we still have work to do there) we do this because we expect some network-wise previsibility and stability of node policies. Do you think “hoping” is acceptable with regards to user funds security ?

Seems like more of an argument for a key validity check, which can optionally check expected future rules too (presumably overlapping with policy in such matters).

If checking for policy compliance is qualified as a real need we should make it easy for application to do so. Not harder by re-implementing checks from Core, and still ensuring to byte-to-byte correctness. How can you optionally check expected future rules if they are not defined somewhere ? Do you think it’s good software engineering practice to steal function from Core directly to achieve this like c-lightning is doing : https://github.com/ElementsProject/lightning/blob/4302afd9a58f0c455bb812b63e9cdf377ebb74d4/bitcoin/signature.c#L214 ?

Sounds like an argument for enabling a chain of txs in testmempoolaccept :)

I don’t think policy/standard can be seen as a monolithic set of rules but instead can be dissociated, you do have adjustable local node policies (-minrelaytxfee, -bytespersigop, …), per-release implementation transaction standards (minimal transaction size, scripts elements, …), mempool DoS limits (BIP 125). AFAIK some of those checks are stateful, i.e depend of previous transactions seen by your mempool. Static rules compliance check shouldn’t be blurred with dynamic ones and that’s not what testmempoolaccept offer. Further how can you prevent your mempool to be sybilled and thus maliciously fail transaction verification ?

Test the highest-fee variant?

What does mean highest-fee variant for a transaction which may be broadcast at an unknown time in the future ? Further testing the highest-fee variant means for a cooperative transactions you have signatures from both parties, how can you prevent a counterparty you don’t trust to not broadcast the highest-fee variant ?

To resume,

LN security model relies on confirmation of time-sensitive transaction those not propagating through the network is a user fund risk.
Propagating is ensured by compliance to standard rules, as enforced by a high chunk of your local tx-relay topology through miner mempools. Given short timelock delay (< 10 blocks) you can’t expect node operator intervention to modify transactions defaulting to conform to standard.
Assuming these standard rules are well-defined (which I think it’s not), higher-layer protocols should incorporate them in their specs. Applications should check their transactions compliance to these rules, both in their testing framework and in production when compliance can’t be known ahead of time (like a witness element provided by counterparty). This evaluation should be fine-grained to avoid a dynamic rule evaluation being blurred with a static one.
Dynamic rule compliance (like mempool min fee) should be anticipated by applications, i.e reacting to a poor-feerate by RBF/CPFP’ing their transaction after few blocks when they see their transactions not confirming.

It’s a bit sad to say that but base layer “standard” rules are somehow part of LN “consensus” rules. Is this a design flaw ? Maybe, but ignoring this would mean to redesign significantly multi-party offchain protocols or any scaling solution. Is Lightning security model well-understood by the LN community itself ? Well that’s work in progress.

I would be glad to have discussion in another meeting if you think there are points we can’t agree on or need to shed more lights on.

luke-jr commented at 4:38 am on June 20, 2020: member

How can we be sure we’re not acting on policy if its scope is not well-defined ? Sounds like a) policy is a moving target but b) be sure you’re not making any assumption on a moving target :) @ariard Policy’s scope is well-defined… it’s any decisions made that aren’t required by the consensus protocol.

Again, how can you say they tolerate non-standard things if you don’t have a standard norm to refer to ? Can you point to those standards you’re mentioning ? How can we check conformity if it’s not defined by some spec or enforced by some code ?

Bitcoin standards are typically published at https://github.com/bitcoin/bips

Ensuring propagation of time-sensitive transaction is critical for this kind of application. Even if Lightning BOLTs define strict transaction and script templates (and honestly we still have work to do there) we do this because we expect some network-wise previsibility and stability of node policies. Do you think “hoping” is acceptable with regards to user funds security ?

It isn’t something you can ensure, though. Hoping your hopes are more than hopes doesn’t really change that.

If checking for policy compliance is qualified as a real need

It’s impossible, so can’t be. All you’re doing here is observing an arbitrary default policy you might run (although by ignoring the user configuration, even that isn’t assured). It tells you nothing about the rest of the networks’ policies.

Static rules compliance check shouldn’t be blurred with dynamic ones

That’s my point. All policy is dynamic. The only static rules are consensus rules (although even those can change with some conditions met).

What does mean highest-fee variant for a transaction which may be broadcast at an unknown time in the future ? Further testing the highest-fee variant means for a cooperative transactions you have signatures from both parties, how can you prevent a counterparty you don’t trust to not broadcast the highest-fee variant ?

If you don’t have the signatures, you can’t ensure you can broadcast it later…?

theuni commented at 1:07 am on June 23, 2020: member

I’m afraid that this isn’t as all-encompassing as it may seem. If the goal is a boolean “yes” or “no” as to whether a spending script would propagate, I don’t think this is enough.

As a quick example, if a scriptSig is not pushonly, it will not fail policy in the interpreter. It will fail above that, inside of IsStandardTx. Arguably, SCRIPT_VERIFY_SIGPUSHONLY could be added to the standard flags, but I believe that other unchecked policies would still remain outside of the interpreter.

If this is to move forward, IMO it needs an audit to guarantee that it’s working as-advertised.

ariard commented at 11:23 pm on June 24, 2020: member

@luke-jr

You gave a formal definition of policy, namely negatively define wrt consensus rules. The material content of such rules is actually what is implemented by Core for a given release. Some of them are standardized in BIPs (but not all of them, like MINIMALIF). A lot of them can’t be changed by user configuration like the script interpreter ones. For the ones which aren’t dependent of a node operator and dynamic network conditions (like mempool feerate) an application should be able to test its transactions compliance with regards to a given version of Core. If you expect a high-level of Core nodes running on the network, that would give you a reasonable expectation on the effective propagability of your transactions.

Now why do we have as standard BIP 125 or BIP 146 and not other Core policy rules ?

That’s my point. All policy is dynamic. The only static rules are consensus rules (although even those can change with some conditions met).

Dynamic with regards to which context ? What the variable(s) making them dynamic ? Beyond the small scope at the choice of node operators, they are bounded with regards to a given release of a given implementation.

ariard commented at 11:34 pm on June 24, 2020: member

As a quick example, if a scriptSig is not pushonly, it will not fail policy in the interpreter. It will fail above that, inside of IsStandardTx. Arguably, SCRIPT_VERIFY_SIGPUSHONLY could be added to the standard flags, but I believe that other unchecked policies would still remain outside of the interpreter.

I agree, beyond a verification of witness compliance you’re transaction may be rejected for a lot of others rules. Like pushonly you mention, MIN_STANDARD_TX_NONWITNESS_SIZE, MAX_STANDARD_TX_WEIGHT among others. Exporting this is mostly a starter. Do you think starting a bitcoinstandard library would be more appropriate ?

If this is to move forward, IMO it needs an audit to guarantee that it’s working as-advertised.

You mean an audit of the script interpreter to acknowledge that standard script flags are working as expected or an audit that on-the-top-application using this library export do have their transactions effectively broadcasting ?

TheBlueMatt commented at 3:29 am on June 26, 2020: member

You mean an audit of the script interpreter to acknowledge that standard script flags are working as expected or an audit that on-the-top-application using this library export do have their transactions effectively broadcasting ?

I believe he means carefully reading all relevant standardness checks (ie starting from ATMP and reading the full function and all the things it calls) and making sure that all those that can be exposed are, and all those that cannot are carefully documented in the new API docs.

theuni commented at 3:11 pm on June 26, 2020: member

Thanks @TheBlueMatt, that’s indeed what I meant.

Either this api is meant to export the policy flags so that 3rd party developers can control what checks are done (I believe this idea has been largely NACK’d), or it’s meant to mirror Core’s exact relay policy. If we attempt the latter but only end up performing a subset of ATMP’s checks, I’m not sure what the real-world utility the library would be.

laanwj removed this from the "Blockers" column in a project

ariard commented at 4:34 pm on August 2, 2020: member

Closing for now, this work necessitates more conceptual discussion on exposing Core’s relay policy and first and foremost if applications should have access to it or not.

ariard closed this on Aug 2, 2020

fanquake added this to the "Plans and discussion" column in a project

DrahtBot locked this on Sep 2, 2022

Export standard Script flags in bitcoinconsensus #18797