Export standard Script flags in bitcoinconsensus

[ariard]

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 lightning/bolts#764 as example of protocol specification directly requiring adherence to standard rules.

[sipa]

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]

@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")

[sipa]

@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.

[ryanofsky]

Concept ACK for bed1e9f. 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]

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]

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]

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]

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

[TheBlueMatt]

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.

[maflcko]

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]

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.

[ryanofsky]

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

[ariard]

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

[ryanofsky]

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

[ryanofsky]

Code review ACK 9f0a6f3. 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]

Code Review ACK 9f0a6f3
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.

[fjahr]

Code review ACK 9f0a6f3

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

[fjahr]

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]

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]

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.

[ariard]

@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]

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?

[ariard]

@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,

1. LN security model relies on confirmation of time-sensitive transaction those not propagating through the network is a user fund risk.
2. 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.
3. 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.
4. 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]

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]

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]

@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]

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]

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]

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.

[ariard]

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.