doc: IsFinalTx comment about nSequence & OP

nothingmuch commented at 8:53 PM on February 7, 2020: contributor

It's somewhat surprising that a transaction's nLockTime field is ignored when all nSequence fields are final, so this change aims to clarify this behavior and cross reference relevant details of OP_CHECKLOCKTIMEVERIFY.

amitiuttarwar commented at 9:11 PM on February 7, 2020: contributor

ACK 5546ed61c65c167ba839df7b5277f8689242ae8b. Sheds light on a confusing corner of the code.

DrahtBot added the label Consensus on Feb 7, 2020

DrahtBot added the label Docs on Feb 7, 2020

in src/script/interpreter.cpp:1748 in 5546ed61c6 outdated

1352 | -    // finalized by setting nSequence to maxint. The
1353 | +    // Finally the nLockTime feature can be disabled in IsFinalTx()
1354 | +    // and thus CHECKLOCKTIMEVERIFY bypassed if every txin has
1355 | +    // been finalized by setting nSequence to maxint. The
1356 |      // transaction would be allowed into the blockchain, making
1357 |      // the opcode ineffective.

ariard commented at 9:30 PM on February 13, 2020:

"This enable a lightweight onchain covenant where you can be safe than constrained output can't be spent until reaching committed nBlockHeight/nBlockTime"

nothingmuch commented at 2:09 PM on February 15, 2020:

I expanded a bit on your suggestion in c4a022d4

p.s. sorry for accidentally clicking resolve

ariard approved

ariard commented at 9:32 PM on February 13, 2020: member

ACK 5546ed6

Thanks for documenting this foresight of OP_CLTV design.

nothingmuch referenced this in commit c4a022d4f7 on Feb 15, 2020

DrahtBot commented at 4:31 AM on February 17, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

in src/consensus/tx_verify.cpp:25 in c4a022d4f7 outdated

  19 | @@ -20,6 +20,15 @@ bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime)
  20 |          return true;
  21 |      if ((int64_t)tx.nLockTime < ((int64_t)tx.nLockTime < LOCKTIME_THRESHOLD ? (int64_t)nBlockHeight : nBlockTime))
  22 |          return true;
  23 | +
  24 | +    // Even if tx.nLockTime isn't satisfied by nBlockHeight/nBlockTime, a
  25 | +    // transaction is still considered final if all nSequence values are final,

michaelfolkson commented at 10:06 PM on May 9, 2020:

nit: You could make clear that by final you mean if the nSequence values are maxed out to 0xffffffff. Perhaps this wording of final is clear to everyone else (in which case ignore).

michaelfolkson commented at 10:09 PM on May 9, 2020: contributor

ACK c4a022d4f75384ba9d6978971fcaeabd072a9acc. Nice addition.

amitiuttarwar commented at 10:13 PM on May 9, 2020: contributor

@nothingmuch could you squash the fixup commit ?

nothingmuch force-pushed on May 10, 2020

nothingmuch commented at 1:28 AM on May 10, 2020: contributor

Done. I've also included @michaelfolkson's suggestion.

nothingmuch force-pushed on May 10, 2020

nothingmuch commented at 1:36 AM on May 10, 2020: contributor

After rereading the diff I added a minor rewording of the comment explaining nLockTime covenants, since I felt it lacked clarity, and force pushed that so that the PR is still squashed at the moment.

nothingmuch force-pushed on May 10, 2020

michaelfolkson commented at 3:51 PM on May 11, 2020: contributor

ACK ae93eb3379604a5ee83242f420bb5431fa238b9e. British English spelling of "behaviour" but not worth another force push to change in my view.

in src/script/interpreter.cpp:1744 in ae93eb3379 outdated

1433 | @@ -1434,16 +1434,22 @@ bool GenericTransactionSignatureChecker<T>::CheckLockTime(const CScriptNum& nLoc
1434 |      if (nLockTime > (int64_t)txTo->nLockTime)
1435 |          return false;
1436 |  
1437 | -    // Finally the nLockTime feature can be disabled and thus
1438 | -    // CHECKLOCKTIMEVERIFY bypassed if every txin has been
1439 | -    // finalized by setting nSequence to maxint. The
1440 | +    // Finally the nLockTime feature can be disabled in IsFinalTx()

amitiuttarwar commented at 5:19 PM on May 15, 2020:

I'm not a huge fan of having function names in the comments, because the code might change over time. eg. theoretically there could be another calling path that also disables, right?

in this case I don't think its a big deal esp bc its consensus code that seems quite unlikely to change

nothingmuch commented at 6:47 PM on May 15, 2020:

FWIW the rationale for adding it was specifically to cross reference these the two locations in the code, as they are more easily understood together.

I can definitely see why this is not worth the risk of stale comments, but [edit to add: assuming it's desired] couldn't think of a simpler way.

Maybe just mentioning CHECKLOCKTIMEVERIFY and SEQUENCE_FINAL is a good compromise, there are few false positives, and those are even less likely to change.

amitiuttarwar commented at 10:00 PM on May 15, 2020:

FWIW the rationale for adding it was specifically to cross reference these the two locations in the code, as they are more easily understood together.

fair enough. makes sense in this case

amitiuttarwar commented at 5:22 PM on May 15, 2020: contributor

ACK ae93eb3379604a5ee83242f420bb5431fa238b9e

one comment, but not worth holding up the PR. its been open for a while and overall this documentation helps clarify some unexpected logic, so def an improvement in my eyes.

theStack commented at 9:14 PM on July 26, 2020: member

Concept ACK, definitely makes sense to document unexpected behaviour in more detail.

nothingmuch commented at 11:09 PM on July 26, 2020: contributor

i fixed both "behaviour" and "the the", will squash & rebase after i reread everything tomorrow

nothingmuch force-pushed on Jul 28, 2020

nothingmuch commented at 2:56 AM on July 28, 2020: contributor

so, despite previous ACKs i couldn't resist rephrasing the paragraph about the locktime covenants, since i could barely understand what i had originally meant by it... squashed and rebased on master

amitiuttarwar commented at 3:35 AM on August 8, 2020: contributor

ACK abc8f3bae2cd9a65e170528d77973307050c5b47.

Hoping this PR can get merged soon!

benthecarman commented at 11:42 PM on September 8, 2020: contributor

ACK abc8f3b

Zero-1729 commented at 3:53 AM on September 9, 2020: contributor

ACK abc8f3b. Looks good.

in src/script/interpreter.cpp:1453 in abc8f3bae2 outdated

1451 | +    // This additional constraint enables a kind of lightweight
1452 | +    // covenant: by using OP_CHECKLOCKTIMEVERIFY an output may
1453 | +    // be unspendable until a specific nBlockHeight/nBlockTime
1454 | +    // committed in the scriptPubKey, as opposed to a spending
1455 | +    // transaction's nLockTime only being constrained by
1456 | +    // signature verification.

MarcoFalke commented at 9:52 AM on April 21, 2021:

I think the Bitcoin Core source code (or interpreter source code) is the wrong place to document what is possible with each opcode. Better places to document this is the BIPs repo (or stackexchange, the mailing list, your private blog or website, ...).

In fact the BIP already mentions several use cases. See https://github.com/bitcoin/bips/blob/master/bip-0065.mediawiki#Motivation . Should the other use-cases be copy-pasted here as well? If a new use-case is found, should it be copied here, too? How much review/maintenance time is this going to consume?

Also, you mention the word covenant, which isn't defined in our source code, nor the BIP text. I think this section adds more confusion for anyone reading the BIP and then this section.

nothingmuch commented at 2:35 PM on April 21, 2021:

That sounds reasonable to me and fits in with the original motivation. @ariard since this was your suggestion do you mind if I remove it?

nothingmuch commented at 3:14 AM on April 24, 2021:

I went ahead and removed this addition.

ariard commented at 5:31 PM on April 26, 2021:

Fine to remove it. IIRC, I did advise you this comment because I was surprised to see nowhere this relation between nLocktime enforcement and finality of inputs documented. At least not in BOLT3, where timeout transactions nSequence are carefully set to non-final.

Overall I agree with @MarcoFalke. Though I would say some mechanism usage towards specific Bitcoin applications are already documented in the codebase. For e.g the carve-out rule in the mempool : https://github.com/bitcoin/bitcoin/blob/683dda2a70e7a210996fa34be23bd0c563971ba9/src/validation.cpp#L801

MarcoFalke dismissed

MarcoFalke commented at 9:54 AM on April 21, 2021: member

Seems fine to better explain how IsFinalTx works, but I am not sure about starting a section about "covenants".

nothingmuch force-pushed on Apr 24, 2021

78051301012 commented at 7:06 PM on April 24, 2021: none

src/script/interpreter.cpp

in src/consensus/tx_verify.cpp:25 in 047c6075ed outdated

  19 | @@ -20,6 +20,15 @@ bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime)
  20 |          return true;
  21 |      if ((int64_t)tx.nLockTime < ((int64_t)tx.nLockTime < LOCKTIME_THRESHOLD ? (int64_t)nBlockHeight : nBlockTime))
  22 |          return true;
  23 | +
  24 | +    // Even if tx.nLockTime isn't satisfied by nBlockHeight/nBlockTime, a
  25 | +    // transaction is still considered final if all nSequence == SEQUENCE_FINAL

ariard commented at 5:31 PM on April 26, 2021:

nit: if all inputs's nSequence

ariard commented at 5:32 PM on April 26, 2021: member

ACK 047c607

nothingmuch force-pushed on Apr 27, 2021

doc: IsFinalTx comment about nSequence & OP_CLTV

It's somewhat surprising that a transaction's nLockTime field is ignored
when all nSequence fields are final, so this change aims to clarify this
behavior and cross reference relevant details of OP_CHECKLOCKTIMEVERIFY.

f9e37f33ce

nothingmuch force-pushed on Apr 27, 2021

nothingmuch requested review from MarcoFalke on Jun 28, 2021

MarcoFalke commented at 7:44 AM on June 29, 2021: member

ACK f9e37f33ce2d8b463a0bcbe7189c9bc5b36530b7

MarcoFalke merged this on Jun 30, 2021

MarcoFalke closed this on Jun 30, 2021

sidhujag referenced this in commit f86d791e0e on Jun 30, 2021

PastaPastaPasta referenced this in commit 21eff66538 on Sep 21, 2021

PastaPastaPasta referenced this in commit 4b6409fe26 on Sep 24, 2021

kittywhiskers referenced this in commit b06e94cba9 on Oct 12, 2021

gwillen referenced this in commit 29a49051ba on Jun 1, 2022

DrahtBot locked this on Aug 18, 2022

doc: IsFinalTx comment about nSequence & OP_CLTV #18096

Conflicts