Skip to content
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.

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

May 2022 Hardfork #499

Merged
merged 15 commits into from
May 5, 2022
Merged

May 2022 Hardfork #499

merged 15 commits into from
May 5, 2022

Conversation

cpacia
Copy link
Contributor

@cpacia cpacia commented Apr 16, 2022

This PR upgrades BCHD to support the May 2022 hardfork

Tasks:

  • Create 64 bit integer and new opcode script flags
  • Create 64 bit integer implementation
  • Create 64 bit integer tests
  • Create opcode implementations
  • Create opcode tests
  • Update params with the activation timestamp and set the script flags after activation
  • Create activation test
  • Sync with hardfork testnet

Spec:

64 Bit Integers: https://gitlab.com/GeneralProtocols/research/chips/-/blob/master/CHIP-2021-02-Bigger-Script-Integers.md
Native Introspection Opcodes: https://gitlab.com/GeneralProtocols/research/chips/-/blob/master/CHIP-2021-02-Add-Native-Introspection-Opcodes.md

@cpacia cpacia changed the title Add Script64BitIntegers script flag May 2022 Hardfork Apr 16, 2022
 This commit adds a flag to the script engine to use 64 bit integers and updates
 the ScriptNum class to optionally support 64 bit integers.

 This is the first part of the May 2022 hardfork.
@cpacia cpacia added the needs review Needs to be reviewed by the maintainers label Apr 18, 2022
Comment on lines +2920 to +2956
tx1 := createSpendTx(outs[0], 0)
builder := txscript.NewScriptBuilder().
AddOp(txscript.OP_1).
AddOp(txscript.OP_ADD).
AddInt64(1152921504606846976).
AddOp(txscript.OP_EQUAL)
redeemScript, err := builder.Script()
if err != nil {
panic(err)
}

addr, err := bchutil.NewAddressScriptHash(redeemScript, &chaincfg.RegressionNetParams)
if err != nil {
panic(err)
}
script, err := txscript.PayToAddrScript(addr)
if err != nil {
panic(err)
}
tx1.TxOut[0].PkScript = script

block.AddTransaction(tx1)
unsortedTxs = append(unsortedTxs, bchutil.NewTx(tx1))

// Spend from tx1
so1 := &spendableOut{
amount: bchutil.Amount(tx1.TxOut[0].Value),
prevOut: wire.OutPoint{
Hash: tx1.TxHash(),
Index: 0,
},
}

tx2 := createSpendTx(so1, 0)
scriptSig, err := txscript.NewScriptBuilder().
AddInt64(1152921504606846975).
AddData(redeemScript).Script()
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah shit sorry. This suggestion (deleted) just came out as a big red block. It just has 3 line changes:

		sixtyBitInt := int64(1152921504606846976)
...
			AddInt64(sixtyBitInt).
...
			AddInt64(sixtyBitInt - 1).

I suggest it because it took me a while to realize the two scripts are connected.

Could add an op_1 op_mul too (or in replacement of add) if you want to exercise both 64 and the re-enabled op together.

@emergent-reasons
Copy link
Contributor

emergent-reasons commented Apr 18, 2022

Needs OP_MUL to be re-enabled / implemented with appropriate overflow mechanic.

(resolved now)

@emergent-reasons
Copy link
Contributor

Do you want me to take a shot at re-documenting these two?

  • scriptNum
  • makeScriptNum

Also do these need a documentation update since there is going to be a boundary between 32 vs 64 script int behavior? e.g. in script_tests.json. I didn't check for raw numbers that may be asserting current/past behavior on 32 bit script ints.

 ["2147483648 0 ADD", "NOP", "P2SH,STRICTENC", "UNKNOWN_ERROR", "arithmetic operands must be in range [-2^31...2^31] "],
 ["-2147483648 0 ADD", "NOP", "P2SH,STRICTENC", "UNKNOWN_ERROR", "arithmetic operands must be in range [-2^31...2^31] "],

@emergent-reasons
Copy link
Contributor

Finished one pass of review.

  • opcode_test.go took me some time to figure out how all the pieces fit together.
  • op_mul which is added now
  • some minor fixes
  • a few places where I wasn't confident about the test because I didn't understand the logic.

@cpacia
Copy link
Contributor Author

cpacia commented Apr 19, 2022

Do you want me to take a shot at re-documenting these two?

  • scriptNum
  • makeScriptNum

Also do these need a documentation update since there is going to be a boundary between 32 vs 64 script int behavior? e.g. in script_tests.json. I didn't check for raw numbers that may be asserting current/past behavior on 32 bit script ints.

 ["2147483648 0 ADD", "NOP", "P2SH,STRICTENC", "UNKNOWN_ERROR", "arithmetic operands must be in range [-2^31...2^31] "],
 ["-2147483648 0 ADD", "NOP", "P2SH,STRICTENC", "UNKNOWN_ERROR", "arithmetic operands must be in range [-2^31...2^31] "],

sure

@emergent-reasons
Copy link
Contributor

emergent-reasons commented Apr 20, 2022

sure

Made a first draft only for scriptnum and makeScriptNum here: #501

It's set so you can make commits directly or if you mention any changes, I can make them.

@@ -6,6 +6,7 @@ package blockchain

import (
"fmt"
"github.com/gcash/bchd/chaincfg"

Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: extra newline.

// for the main network.
var testNet4GenesisMerkleRoot = genesisMerkleRoot

// testNet3GenesisBlock defines the genesis block of the block chain which
Copy link
Contributor

Choose a reason for hiding this comment

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

testNet4GenesisBlock

@@ -611,6 +623,97 @@ var TestNet3Params = Params{
SlpAddressPrefix: "slptest",
}

// TestNet4Params defines the network parameters for the test Bitcoin network
// (version 3). Not to be confused with the regression test network, this
Copy link
Contributor

Choose a reason for hiding this comment

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

(version 4).

// Chain parameters
GenesisBlock: &testNet4GenesisBlock,
GenesisHash: &testNet4GenesisHash,
PowLimit: testNet3PowLimit,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have another variable for the PowLimit? Odd to use testNet3 values here.

Copy link
Contributor

@zquestz zquestz left a comment

Choose a reason for hiding this comment

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

This looks amazing. Thank you for doing this work @cpacia.

Have we synced this against both testnet3 and testnet4?

@emergent-reasons
Copy link
Contributor

This looks amazing. Thank you for doing this work @cpacia.

Have we synced this against both testnet3 and testnet4?

im_uname says it has been tested against testnet4. The problem is that it didn't fail before op_mul was implemented in this PR. So it probably still needs to be synced against some op_mul tx.

@cpacia
Copy link
Contributor Author

cpacia commented Apr 30, 2022

Yeah it synced to testnet4 but it was odd that it did because OP_MUL wasn't activated when I did it. Are there OP_MUL transactions on that chain?

@emergent-reasons
Copy link
Contributor

Yeah it synced to testnet4 but it was odd that it did because OP_MUL wasn't activated when I did it. Are there OP_MUL transactions on that chain?

These are supposed to have op_mul:

@cpacia cpacia removed the needs review Needs to be reviewed by the maintainers label May 5, 2022
@cpacia cpacia merged commit 58741b0 into master May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants