Contract name length for Clarity 2

[obycode]

It was discovered via a discussion on Discord that SIP 005 specifies that a contract name can be 128 characters

The contract principal variant of the principal post-condition field is encoded as follows: A 1-byte type prefix of 0x03 The 1-byte version of the standard principal that issued the contract The 20-byte Hash160 of the standard principal that issued the contract A 1-byte length of the contract name, up to 128 The contract name

However the parser limits the contract name to 40 characters:

pub const CONTRACT_MIN_NAME_LENGTH: usize = 1;
pub const CONTRACT_MAX_NAME_LENGTH: usize = 40;

    pub static ref CONTRACT_NAME_REGEX: String = format!(
        r#"([a-zA-Z](([a-zA-Z0-9]|[-_])){{{},{}}})"#,
        CONTRACT_MIN_NAME_LENGTH - 1,
        CONTRACT_MAX_NAME_LENGTH - 1
    );

For Clarity 2, do we want to keep the 40 character limit or go back to the 128 character limit specified in the SIP?

[jcnelson]

I'm in favor of keeping the 40 character limit and updating SIP 005.

[jcnelson]

N.B. the transaction wire format allows 128 characters, so that should be fixed at 40.

[zone117x]

Note this issue is more than just a documentation discrepancy. Contract call transactions with args containing large contract names (over 40 bytes) can be successfully included in a block.

For an example, see SP2C2YFP12AJZB4MABJBAJ55XECVS7E4PMMZ89YZR.aip10-arkadiko-update-tvl-liquidation-ratio in https://explorer.stacks.co/txid/0xf402ee582892c48679e6bbfbdc1f12a1da5f1ebf705dfacd1982ad70296490f2?chain=mainnet

[obycode]

This will be caught and reported as an error by the new parser.

[jcnelson]

@obycode Can I confirm that this was fixed by the v2 parser?

[obycode]

The new parser catches and reports an error when a contract principal in the code has a contract name longer than 40 characters - see https://github.com/stacks-network/stacks-blockchain/blob/3a24af692776c822f0ddb238c9362d09c6f6d12d/clarity/src/vm/ast/parser/v2/mod.rs#L2782-L2801

The parser can't do anything to prevent a contract from being published with a long name, or passing a contract principal with a long name as a contract call argument. I'm not sure if those are handled properly now.

[jcnelson]

The parser can't do anything to prevent a contract from being published with a long name, or passing a contract principal with a long name as a contract call argument. I'm not sure if those are handled properly now.

I don't think they are. I'll escalate this as a 2.1 issue.

[jcnelson]

@zone117x I wish I saw that message earlier and comprehended its significance.

I just noticed that my 2.1 node does not boot. In fact, it gets stuck when trying to process the microblock stream that contains that every transaction: a contract-call that contains that very principal argument.

[obycode]

The parser change here should only be in the v2 parser, which should only be effective in epoch 2.1. Did this length check also get added to the v1 parser? I can check the code shortly.

[jcnelson]

The bug isn't in the parser. It's in 2da31c5c99, where you changed CONTRACT_NAME_REGEX_STRING in clarity/src/vm/representations.rs to only accept strings of length at most CONTRACT_MAX_NAME_LENGTH.

[obycode]

I see. Thanks Jude, and sorry about that. The change to the regex for the ContractName is not even necessary for the 40 character limit to be caught in 2.1. I had made that change to simplify, because there were two different regex strings being used for contract names, and I missed the fact that this was a problem.

Thanks for your fix. It looks good to me.

[jcnelson]

No worries! This is why we're doing the genesis test before -rc1 -- to shake out any inconsistencies like this :)

I'll keep my eye on the node and see if the change gets it unstuck without breaking something else.

[jcnelson]

Notes from the blockchain meeting:

• This can be fixed in tooling. stacks.js and microstacks.js can report a warning (or error out) if they try to instantiate a contract principal literal with a name longer than 40 characters.

• It's a little late to fix this in 2.1, so we'll just bump this to the next hard fork.

• @obycode will add a test case to verify that the buggy behavior is preserved in 2.1.

[obycode]

PR #3462 adds the test case.