Skip to content

Implement 64 bit arithmetic op codes in the Script interpreter #29221

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
Closed
wants to merge 2 commits into from
Closed

Implement 64 bit arithmetic op codes in the Script interpreter #29221

wants to merge 2 commits into from

Conversation

Christewart
Copy link
Contributor

This PR adds 64 bit arithmetic op codes to the Script interpreter.

This PR corresponds with this BIP proposal: bitcoin/bips#1538

This work is heavily borrowed from the elements implementation of 64 bit op codes: ElementsProject/elements#1020

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 10, 2024 •
edited
Loading

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK moonsettler, tiero, hsjoberg

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #30076 (test: fix MiniWallet script-path spend (missing parity bit in leaf version) by theStack)
  • #29371 (test: Add leaf_version parameter to taproot_tree_helper() by Christewart)
  • #29270 (Implement OP_CHECKSIGFROMSTACK(VERIFY) by reardencode)
  • #29269 (Add OP_INTERNALKEY for Tapscript by reardencode)
  • #29247 (Reenable OP_CAT by 0xBEEFCAF3)
  • #28710 (Remove the legacy wallet and BDB dependency by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@Christewart Christewart changed the title Implement OP_ADD64, OP_SUB64, OP_MUL64, OP_DIV64, OP_LESSTHAN64, OP_L… Implement 64 bit arithmetic op codes to the Script interpreter Jan 10, 2024
@Christewart Christewart changed the title Implement 64 bit arithmetic op codes to the Script interpreter Implement 64 bit arithmetic op codes in the Script interpreter Jan 10, 2024
@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/20360373986

This was referenced Jan 11, 2024
Closed
Closed
Merged
Closed
Open
@moonsettler
Copy link

Concept ACK. i see no reason for this not to be bundled with any future amount introspection soft fork.

@tiero
Copy link

tiero commented Jan 12, 2024

Concept ACK

@hsjoberg
Copy link
Contributor

Concept ACK. This is great!

@DrahtBot DrahtBot mentioned this pull request Jan 15, 2024
Draft
@Christewart Christewart mentioned this pull request Jan 17, 2024
Closed
This was referenced Jan 18, 2024
Closed
Closed
@Christewart Christewart force-pushed the 64bit-arith branch 2 times, most recently from 4037f19 to c58bd68 Compare February 1, 2024 20:07
@Christewart Christewart mentioned this pull request Feb 1, 2024
Closed
This was referenced Feb 2, 2024
Closed
Closed
@Christewart Christewart mentioned this pull request Feb 2, 2024
Open
@DrahtBot DrahtBot removed the CI failed label Feb 3, 2024
@Christewart Christewart force-pushed the 64bit-arith branch 3 times, most recently from 3ffb1bc to 86b1069 Compare February 3, 2024 20:35
@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 6, 2024

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/22345188040

@DrahtBot DrahtBot removed the CI failed label Mar 7, 2024
This was referenced Mar 7, 2024
Closed
Closed
Merged
Closed
@DrahtBot DrahtBot mentioned this pull request Mar 13, 2024
Merged
@DrahtBot DrahtBot mentioned this pull request May 23, 2024
Merged
@Christewart
Add SigVersion parameter to IsOpSuccess()
cc16fca 
Rework IsOpSuccess() to use SigVersion rather than leaf_version

Use switch based impl for IsOpSuccess()

Remove default in switch statement

Fix compiler warning

Move SigVersion to sigversion.h

Fix includes

try to fix compile

Add sigversion.h

Add include guards
@Christewart
Implement OP_ADD64, OP_SUB64, OP_MUL64, OP_DIV64, OP_LESSTHAN64, OP_L…
2c40ae0 
…ESSTHANOREQUAL64, OP_GREATERTHAN64, OP_GREATERTHANOREQUAL64, OP_SCRIPTNUMTOLE64, OP_LE64TOSCRIPTNUM, OP_LE32TOLE64

Remove liquid args

WIP

Get simple OP_1 functional test case working

Get tests for arithmetic and comparison opcodes working

Get all functional tests passing

Rename test case to Arithmetic64bitTest

Rename file to feature_64bit_arithmetic_opcodes.py, add it to test_runner.py

Get tests passing in feature_taproot.py

Remove unused push_le4

Revert test fixture setup

Cleanup

Fix linting

test: Add leaf_version parameter to taproot_tree_helper()

Fix bug

Fix bugs

Fix compile

Fix missing sigversion checks

Fix htole64 -> htole64_internal due to bitcoin#29263

Add negative test case for OP_LE64TOSCRIPTNUM

Use OP_EQUAL rather than OP_EQUALVERIFY

Fix rebase
@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

@Christewart
Copy link
Contributor Author

Too speculative for now https://delvingbitcoin.org/t/64-bit-arithmetic-soft-fork/397/53

@bitcoin bitcoin locked and limited conversation to collaborators Aug 23, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
CI failed Consensus Needs rebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Christewart @DrahtBot @moonsettler @tiero @hsjoberg @ajtowns