WIP - BitcoinCash Upgrade 9 Implementation #528

OPReturnCode · 2023-06-18T00:41:39Z

This is an attempt to implement the latest BCH consensus rules into BCHD.

Upgrade 9 requires includes the following changes:

Add ChipNet Support
Restrict Transaction Version (Tiny change)
Add New Minimum Transaction Size
Implement P2SH32 (This relies on gcash/bchutil - the changes are implemented there but not pushed yet)
Implement CashTokens (Biggest task by far)

Please note that the items that are checked as complete in the above todo list, might still need some light modifications and slight touches by the time the rest of the todo list is complete.

zquestz · 2023-06-19T03:56:13Z

blockchain/indexers/addrindex.go

+	// TODO: Since we updated the size, please note that we need to either:
+	// a) detect old indexes and force them to be rebuilt
+	// b) write a migration of the existing index
+	addrKeySize = 1 + 32


Should this be increased further to support larger valid cashaddr addresses?

I assume this is referencing the size of the payload? If so, some examples for reference:

https://gitlab.com/cash-accounts/specification/-/blob/master/SPECIFICATION.md#payment-data-types

zquestz · 2023-06-19T03:57:25Z

chaincfg/params.go

@@ -262,7 +263,7 @@ var MainNetParams = Params{
 	Name:        "mainnet",
 	Net:         wire.MainNet,
 	DefaultPort: "8333",
-	DNSSeeds: []DNSSeed{
+	DNSSeeds: []DNSSeed{ // TODO change this to proper dns seed


Just a note, I run seed.bchd.cash, and as we finish changes will update that codebase.

zquestz · 2023-06-19T03:58:58Z

This looks like a very solid start to the required changes. Thanks so much for the effort @OPReturnCode!

A60AB5450353F40E · 2023-06-19T13:33:53Z

bchrpc/server.go

@@ -1017,7 +1017,7 @@ func (s *GrpcServer) GetAddressUnspentOutputs(ctx context.Context, req *pb.GetAd
 			matchAddr := ""

 			switch typedAddr := addrs[0].(type) {
-			case *bchutil.AddressPubKeyHash, *bchutil.AddressScriptHash:
+			case *bchutil.AddressPubKeyHash, *bchutil.AddressScriptHash, *bchutil.AddressScriptHash32:


would it be too much trouble to make naming consistent so the old ver. gets called bchutil.AddressScriptHash20, so some future people have an easier time knowing what's what

(not opreturn - drive by opinion - but the problem with this type of preferential change is that it introduces breaking changes across all forks and branches where they are not strictly necessary. Maybe reasonable here but the change is also probably not that valuable in context.)

I agree with emergent-reasons. I usually try to keep the changes as minimal as possible.

A60AB5450353F40E · 2023-06-19T13:34:56Z

blockchain/compress.go

+func isScriptHash32(script []byte) (bool, []byte) {
+	if len(script) == 35 && script[0] == txscript.OP_HASH256 &&
+		script[1] == txscript.OP_DATA_32 &&
+		script[22] == txscript.OP_EQUAL {


is this a bug? should be script[34], right?

A60AB5450353F40E · 2023-06-19T13:41:44Z

chaincfg/params.go

@@ -288,6 +289,8 @@ var MainNetParams = Params{

 	CosmicInflationActivationTime: 1652616000,

+	Upgrade9ForkHeight: 792772, // 000000000000000002B678C471841C3E404EC7AE9CA9C32026FE27EB6E3A1ED1


not sure whether you use this with > or >=, just to double-check: 772 was the last block with old rules, 773 is the first block with new rules

In rpcserver, mempool and process, all use >

e.g.

if height > s.cfg.ChainParams.Upgrade9ForkHeight { upgrade9Active = true }

A60AB5450353F40E · 2023-06-19T13:58:38Z

txscript/standard.go

+	ScriptHashTy                      // Pay to script hash.
+	ScriptHash32Ty                    // Pay to script hash 32
+	MultiSigTy                        // Multi signature.
+	NullDataTy                        // Empty data-only (provably prunable).


just to clarify the comment, what does it mean "empty"? any bytecode that starts with 0x6a (OP_RETURN) is provably prunable - even it has some sats or has some bytecode after the 0x6a

not important here, but it will be relevant for UTXO commitments one day, just to take note: https://bitcoincashresearch.org/t/chip-2021-07-utxo-fastsync/502#utxo-commitment-format-7

emergent-reasons · 2023-06-19T18:13:46Z

chaincfg/params.go

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

+var ChipNetParams = Params{


Totally unnecessary and not desirable if the intent is to have clear static values for all the chain params but... if you want it to be more immediately understandable for the reader, you could move chipnet after testnet 4 and then reference almost all of the testnet 4 values as the chipnet values.

e.g.

BIP0034Height: 2, // --> BIP0034Height: TestNet4Params.BIP0034Height,

I usually like things to be as explicit as possible but it's just opinion and not a hard one. Specially in this case.
So yeah I think it makes sense to change that. It will also be a bit more consistent with the rest of the code.

emergent-reasons · 2023-06-19T19:02:18Z

chaincfg/params.go

@@ -288,6 +289,8 @@ var MainNetParams = Params{

 	CosmicInflationActivationTime: 1652616000,

+	Upgrade9ForkHeight: 792772, // 000000000000000002B678C471841C3E404EC7AE9CA9C32026FE27EB6E3A1ED1


Is there a reason to define this only on mainnet?

No, it's just something to be added. I'll add it.

emergent-reasons · 2023-06-19T19:14:17Z

blockchain/validate.go

+
+	if magneticAnomalyActive && serializedTxSize < MagneticAnomalyMinTransactionSize {
+		minTxSize := MagneticAnomalyMinTransactionSize
+		if upgrade9Active {


I think in rational cases this chained logic works. However, to be safe, I would personally put it outside as its own test. For example, if for some crazy reason someone runs the engine with magneticAnomalyActive disabled, then the chained logic would not detect a tx too small for upgrade9.

🤷 It doesn't matter in practical terms, right?

I get that it makes the ordering explicit. If that's the intent, then maybe a note about ordering would be nice.

emergent-reasons · 2023-06-19T19:42:13Z

I would want to see tests for each of the checked features before considering them complete. Also it would be very nice to have that sorted before going into the depths of the CT implementation and tests so that the diffs going forward can be specific to CT.

emergent-reasons · 2023-06-19T19:55:59Z

bchrpc/server.go

@@ -1017,7 +1017,7 @@ func (s *GrpcServer) GetAddressUnspentOutputs(ctx context.Context, req *pb.GetAd
 			matchAddr := ""

 			switch typedAddr := addrs[0].(type) {
-			case *bchutil.AddressPubKeyHash, *bchutil.AddressScriptHash:
+			case *bchutil.AddressPubKeyHash, *bchutil.AddressScriptHash, *bchutil.AddressScriptHash32:


@OPReturnCode is this the change that requires an update to bchutil?

Works with the branch from opreturn's fork of bchutil.

emergent-reasons · 2023-06-19T20:02:14Z

blockchain/fullblocktests/generate.go

+// payToScriptHashScript32 returns a standard pay-to-script-hash for the provided
+// redeem script.
+func payToScriptHashScript32(redeemScript []byte) []byte {
+	redeemScriptHash := bchutil.Hash256(redeemScript)


I think this is another place that needs the updated bchutil. Leaving a note to come back and check when that is available.

not available yet in the branch I have from opreturn's fork of bchutil.

I assume it will be something like this in a new hash256.go?

// Hash256 calculates the hash sha256(sha256(b)). func Hash256(buf []byte) []byte { return calcHash(calcHash(buf, sha256.New()), sha256.New()) }

emergent-reasons · 2023-06-19T20:13:08Z

txscript/pkscript.go

I can't figure out how to leave a comment on github on a line that's not part of the review. However, I think at least around line 193, additional logic is needed for whether to hash the script with hash160 or hash256.

Might be worth a global grep of hash160 to see if there might be others.

emergent-reasons · 2023-06-19T20:19:28Z

txscript/standard.go

@@ -355,6 +358,13 @@ func payToScriptHashScript(scriptHash []byte) ([]byte, error) {
 		AddOp(OP_EQUAL).Script()
 }

+// payToScriptHashScript creates a new script to pay a transaction output to a


Suggested change

// payToScriptHashScript creates a new script to pay a transaction output to a

// payToScriptHashScript32 creates a new script to pay a transaction output to a

A60AB5450353F40E · 2024-01-26T18:58:57Z

wire/cashtokens.go

+	HAS_COMMITMENT_LENGTH = 0x40
+	RESERVED_BIT          = 0x80
+
+	BASE_TOEN_DATA_LENGTH = 1 + 32 + 1


typo "TOEN"?

A60AB5450353F40E · 2024-01-26T19:00:09Z

wire/cashtokens.go

+
+		scriptLengthCount -= (1 + 32 + 1) //PREFIX_BYTE + CategoryID + BitField
+
+		if tokenData.IsValidBitfield() { // Raise error if false?


yeah TX should fail if bitfield is not one of allowed combinations

zquestz · 2024-05-06T08:55:53Z

Any update on the progress here? Any plans to support ABLA?

OPReturnCode added 4 commits June 17, 2023 17:16

Add chipnet support

d9a8a2f

Add CT opcodes and rules - WIP

6c347a6

modify tx size limit - upgrade9

7fcf552

Add P2SH32

9dfc103

zquestz requested review from cpacia and zquestz June 19, 2023 03:55

zquestz reviewed Jun 19, 2023

View reviewed changes

A60AB5450353F40E reviewed Jun 19, 2023

View reviewed changes

emergent-reasons reviewed Jun 19, 2023

View reviewed changes

Klakurka mentioned this pull request Jun 19, 2023

Switch BCH from BCHD to Fulcrum PayButton/paybutton-server#412

Open

Add CashTokens core logic

723c4ed

A60AB5450353F40E reviewed Jan 26, 2024

View reviewed changes

wire/cashtokens.go

HAS_COMMITMENT_LENGTH = 0x40

RESERVED_BIT = 0x80

BASE_TOEN_DATA_LENGTH = 1 + 32 + 1

Copy link

A60AB5450353F40E Jan 26, 2024

Choose a reason for hiding this comment

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

typo "TOEN"?

A60AB5450353F40E reviewed Jan 26, 2024

View reviewed changes

Add more tests and some fixes

41fe76b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP - BitcoinCash Upgrade 9 Implementation #528

WIP - BitcoinCash Upgrade 9 Implementation #528

OPReturnCode commented Jun 18, 2023 •

edited

zquestz Jun 19, 2023

emergent-reasons Jun 19, 2023

zquestz Jun 19, 2023

zquestz commented Jun 19, 2023

A60AB5450353F40E Jun 19, 2023

emergent-reasons Jun 19, 2023

OPReturnCode Jul 20, 2023

A60AB5450353F40E Jun 19, 2023

A60AB5450353F40E Jun 19, 2023

emergent-reasons Jun 19, 2023

A60AB5450353F40E Jun 19, 2023

emergent-reasons Jun 19, 2023

OPReturnCode Jul 20, 2023

emergent-reasons Jun 19, 2023

OPReturnCode Jul 20, 2023

emergent-reasons Jun 19, 2023 •

edited

emergent-reasons Jun 19, 2023

emergent-reasons commented Jun 19, 2023

emergent-reasons Jun 19, 2023

emergent-reasons Jun 20, 2023

emergent-reasons Jun 19, 2023

emergent-reasons Jun 20, 2023

emergent-reasons Jun 20, 2023

emergent-reasons Jun 19, 2023

emergent-reasons Jun 19, 2023

A60AB5450353F40E Jan 26, 2024

A60AB5450353F40E Jan 26, 2024

zquestz commented May 6, 2024

		@@ -288,6 +289,8 @@ var MainNetParams = Params{

		CosmicInflationActivationTime: 1652616000,

		Upgrade9ForkHeight: 792772, // 000000000000000002B678C471841C3E404EC7AE9CA9C32026FE27EB6E3A1ED1

	// payToScriptHashScript creates a new script to pay a transaction output to a
	// payToScriptHashScript32 creates a new script to pay a transaction output to a


		scriptLengthCount -= (1 + 32 + 1) //PREFIX_BYTE + CategoryID + BitField

		if tokenData.IsValidBitfield() { // Raise error if false?

WIP - BitcoinCash Upgrade 9 Implementation #528

Are you sure you want to change the base?

WIP - BitcoinCash Upgrade 9 Implementation #528

Conversation

OPReturnCode commented Jun 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zquestz commented Jun 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emergent-reasons Jun 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emergent-reasons commented Jun 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zquestz commented May 6, 2024

OPReturnCode commented Jun 18, 2023 •

edited

emergent-reasons Jun 19, 2023 •

edited