Skip to content

chore: set config params for staging-ignition and testnet#16976

Merged
PhilWindle merged 1 commit intonextfrom
mitch/tmnt-258-configuration
Sep 17, 2025
Merged

chore: set config params for staging-ignition and testnet#16976
PhilWindle merged 1 commit intonextfrom
mitch/tmnt-258-configuration

Conversation

@just-mitch
Copy link
Collaborator

@just-mitch just-mitch added the hotfix A PR/issue that needs to be cherrypicked back to the RC label Sep 11, 2025
Copy link
Contributor

@alexghr alexghr left a comment

Choose a reason for hiding this comment

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

Looks great but the large numbers need to either be strings or defined using bigint powers (e.g. 10n ** 18n)

Comment on lines +109 to +111
slashAmountSmall: BigInt(2_000e18),
slashAmountMedium: BigInt(10_000e18),
slashAmountLarge: BigInt(50_000e18),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not safe. This would convert 2000e18 from a number to a bigint but 2000e18 is greater than Number.MAX_SAFE_INTEGER so it'll overflow before it is converted to bigint

Comment on lines +123 to +126
localEjectionThreshold: BigInt(196_000e18),

ejectionThreshold: BigInt(100_000e18),
activationThreshold: BigInt(200_000e18),
Copy link
Contributor

Choose a reason for hiding this comment

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

As above


slashingRoundSizeInEpochs: 4,
slashingLifetimeInRounds: 40,
slashingExecutionDelayInRounds: 20,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
slashingExecutionDelayInRounds: 20,
slashingExecutionDelayInRounds: 28,

Copy link
Contributor

Choose a reason for hiding this comment

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

We talked about having a 3 day slashing execution delay and a 5 day exit delay.

slashingExecutionDelayInRounds: 20,
slashAmountSmall: BigInt(2_000e18),
slashAmountMedium: BigInt(10_000e18),
slashAmountLarge: BigInt(50_000e18),
Copy link
Contributor

Choose a reason for hiding this comment

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

slashAmountLarge: BigInt(50_000e18),

I think this is fine for Ignition where there is no txs, but for Alpha this slashAmountLarge would need to be close to 100% of the stake.

slashMinPenaltyPercentage: 0.5,
slashMaxPenaltyPercentage: 2.0,
slashInactivityTargetPercentage: 0.7,
slashInactivityConsecutiveEpochThreshold: 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we set this to two to test it out?

slashUnknownPenalty: BigInt(2_000e18),
slashBroadcastedInvalidBlockPenalty: BigInt(10_000e18),
slashMaxPayloadSize: 50,
slashGracePeriodL2Slots: 32 * 2, // Two epochs from genesis
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
slashGracePeriodL2Slots: 32 * 2, // Two epochs from genesis
slashGracePeriodL2Slots: 32 * 4, // One round from genesis

slashBroadcastedInvalidBlockPenalty: BigInt(10_000e18),
slashMaxPayloadSize: 50,
slashGracePeriodL2Slots: 32 * 2, // Two epochs from genesis
slashOffenseExpirationRounds: 8,
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: is there any point setting this to far above the slashingOffsetInRounds?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should take input from @spalladino here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just debugging. Any offenses still in the store can be queried via the node admin API.

proposeConfig: {
lockDelay: 60n * 60n * 24n,
lockAmount: DefaultL1ContractsConfig.activationThreshold * 100n,
lockDelay: 60n * 24n * 60n * 60n, // 60 days
Copy link
Contributor

Choose a reason for hiding this comment

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

10 years

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For testnet? Why?

@just-mitch just-mitch force-pushed the mitch/tmnt-258-configuration branch 3 times, most recently from fa8a445 to 6c3986b Compare September 16, 2025 11:01
@@ -125,31 +125,32 @@ const StagingPublicGovernanceConfiguration = {

const TestnetGovernanceConfiguration = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the values are mostly sane, but would have preferred seeing it not explode in tests first (when trying to update some of these in the #16988 I ran into plenty things)

slashAmountLarge: BigInt(50e18),
activationThreshold: 100n * 10n ** 18n,
ejectionThreshold: 50n * 10n ** 18n,
localEjectionThreshold: 98n * 10n ** 18n,
Copy link
Contributor

Choose a reason for hiding this comment

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

None of these values (except slashAmountLarge) matches what we have in the issue TMNT-263: Rollup Configuration) so don't think they are running any tests with the new values at all. (I had issue with that).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They were getting set explicitly over in the chain l2 config, but I will update here as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we change the defaults much here you will see an insane amount of tests failing because many of them are using hard coded values 😬

@just-mitch just-mitch force-pushed the mitch/tmnt-258-configuration branch 3 times, most recently from cd84e6e to 4799590 Compare September 16, 2025 23:06
a: 1000,
k: 1000000,
minimum: 100000,
k: 100000,
Copy link
Contributor

@LHerskind LHerskind Sep 17, 2025

Choose a reason for hiding this comment

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

The existing value was the correct here. It has now been invalidated. All of the reward configs really should just be the same so there is only one and that one would be the TestnetRewardBoostConfig. I will update that.

@LHerskind LHerskind force-pushed the mitch/tmnt-258-configuration branch from 4799590 to f2bee27 Compare September 17, 2025 09:16
@LHerskind
Copy link
Contributor

I pushed an update to the cofngis here, and will be running it on a solidity branch to ensure that the config make it behaves correctly.

Copy link
Contributor

This stack of pull requests is managed by Graphite. Learn more about stacking.

@LHerskind
Copy link
Contributor

I pushed an update to the cofngis here, and will be running it on a solidity branch to ensure that the config make it behaves correctly.

There are some failing tests but they are all related to the configurations changing and some slashing tests, e.g., empire vs. tally and previously having run with 0 delay.

Copy link
Contributor

@LHerskind LHerskind left a comment

Choose a reason for hiding this comment

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

I have updated the reward booster configuration such that there will be only one. It does not make much sense to me that there are actually multiple different ones between networks.

@LHerskind
Copy link
Contributor

I pushed an update to the cofngis here, and will be running it on a solidity branch to ensure that the config make it behaves correctly.

There are some failing tests but they are all related to the configurations changing and some slashing tests, e.g., empire vs. tally and previously having run with 0 delay.

Will be adding fixes and moving this more into the solidity tests as well in #16988

@PhilWindle PhilWindle added this pull request to the merge queue Sep 17, 2025
Merged via the queue into next with commit 3bf6083 Sep 17, 2025
15 checks passed
@PhilWindle PhilWindle deleted the mitch/tmnt-258-configuration branch September 17, 2025 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hotfix A PR/issue that needs to be cherrypicked back to the RC

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants