Skip to content

doc: IsFinalTx comment about nSequence & OP_CLTV #18096

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
maflcko merged 1 commit into from
Jun 30, 2021
Merged

doc: IsFinalTx comment about nSequence & OP_CLTV #18096

maflcko merged 1 commit into from
Jun 30, 2021

Conversation

@nothingmuch
Copy link
Contributor

@nothingmuch nothingmuch commented Feb 7, 2020

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
Copy link
Contributor

amitiuttarwar commented Feb 7, 2020

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

ariard
ariard approved these changes Feb 13, 2020
View reviewed changes
Copy link

@ariard ariard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 5546ed6

Thanks for documenting this foresight of OP_CLTV design.

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 17, 2020 •
edited
Loading

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

Conflicts

No conflicts as of last run.

@DrahtBot DrahtBot mentioned this pull request Feb 17, 2020
Closed
michaelfolkson
michaelfolkson reviewed May 9, 2020
View reviewed changes
Copy link

@michaelfolkson michaelfolkson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK c4a022d4f75384ba9d6978971fcaeabd072a9acc. Nice addition.

@amitiuttarwar
Copy link
Contributor

amitiuttarwar commented May 9, 2020

@nothingmuch could you squash the fixup commit ?

@nothingmuch
Copy link
Contributor Author

nothingmuch commented May 10, 2020

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

@nothingmuch
Copy link
Contributor Author

nothingmuch commented May 10, 2020

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.

@michaelfolkson
Copy link

michaelfolkson commented May 11, 2020

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

amitiuttarwar
amitiuttarwar reviewed May 15, 2020
View reviewed changes
Copy link
Contributor

@amitiuttarwar amitiuttarwar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

theStack commented Jul 26, 2020

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

@nothingmuch
Copy link
Contributor Author

nothingmuch commented Jul 26, 2020

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

@nothingmuch
Copy link
Contributor Author

nothingmuch commented Jul 28, 2020

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
Copy link
Contributor

amitiuttarwar commented Aug 8, 2020

ACK abc8f3bae2cd9a65e170528d77973307050c5b47.

Hoping this PR can get merged soon!

@benthecarman
Copy link
Contributor

benthecarman commented Sep 8, 2020

ACK abc8f3b

@Zero-1729
Copy link
Contributor

Zero-1729 commented Sep 9, 2020

ACK abc8f3b. Looks good.

maflcko
maflcko previously requested changes Apr 21, 2021
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

78051301012
78051301012 reviewed Apr 24, 2021
View reviewed changes
Copy link

@78051301012 78051301012 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/script/interpreter.cpp

ariard
ariard reviewed Apr 26, 2021
View reviewed changes
Copy link

@ariard ariard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 047c607

@nothingmuch
doc: IsFinalTx comment about nSequence & OP_CLTV
f9e37f3 
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.
@nothingmuch nothingmuch requested a review from maflcko June 28, 2021 18:30
@maflcko
Copy link
Member

maflcko commented Jun 29, 2021

ACK f9e37f3

@maflcko maflcko merged commit e1a13f1 into bitcoin:master Jun 30, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 30, 2021
Merge bitcoin#18096: doc: IsFinalTx comment about nSequence & OP_CLTV
f86d791 
f9e37f3 doc: IsFinalTx comment about nSequence & OP_CLTV (Yuval Kogman)

Pull request description:

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

ACKs for top commit:
  MarcoFalke:
    ACK f9e37f3

Tree-SHA512: 88460dacbe4b8115fb1948715f09b21d4f34ba1da9e88d52f0b774a969f845e9eddc5940e7fee66eacdd3062dc40d6d44c3f282b0e5144411fd47eb2320b44f5
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 24, 2021
@PastaPastaPasta
Merge bitcoin#18096: doc: IsFinalTx comment about nSequence & OP_CLTV
4b6409f 
f9e37f3 doc: IsFinalTx comment about nSequence & OP_CLTV (Yuval Kogman)

Pull request description:

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

ACKs for top commit:
  MarcoFalke:
    ACK f9e37f3

Tree-SHA512: 88460dacbe4b8115fb1948715f09b21d4f34ba1da9e88d52f0b774a969f845e9eddc5940e7fee66eacdd3062dc40d6d44c3f282b0e5144411fd47eb2320b44f5
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@apoelstra
Merge e1a13f1 into merged_master (Bitcoin PR bitcoin/bitcoin#18096)
29a4905
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@maflcko maflcko Awaiting requested review from maflcko

4 more reviewers

@amitiuttarwar amitiuttarwar amitiuttarwar left review comments

@michaelfolkson michaelfolkson michaelfolkson left review comments

@ariard ariard ariard approved these changes

@78051301012 78051301012 78051301012 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Consensus Docs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@nothingmuch @amitiuttarwar @DrahtBot @michaelfolkson @theStack @benthecarman @Zero-1729 @maflcko @ariard @78051301012