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

Modify other constants based on decreased block time #3959

Closed
rootulp opened this issue Oct 10, 2024 · 1 comment
Closed

Modify other constants based on decreased block time #3959

rootulp opened this issue Oct 10, 2024 · 1 comment
Assignees

Comments

@rootulp
Copy link
Collaborator

rootulp commented Oct 10, 2024

Context

Originally posted by @jcstein in #3953 (comment)

Problem

The constant GoalBlockTime is used:

  1. To determine Mempool TTLDuration
  2. To determine Evidence MaxAgeNumBlocks
  3. To determine x/ibc MaxExpectedTimePerBlock

#3882 proposes decreasing block times from 12 to 6 seconds.

Proposal

Re-evaluate if those constants need to be modified if we decrease block time from 12 to 6 seconds.

@rootulp
Copy link
Collaborator Author

rootulp commented Oct 10, 2024

Mempool TTL duration

// TTLDuration, if non-zero, defines the maximum amount of time a transaction
// can exist for in the mempool.
//
// Note, if TTLNumBlocks is also defined, a transaction will be removed if it
// has existed in the mempool at least TTLNumBlocks number of blocks or if it's
// insertion time into the mempool is beyond TTLDuration.
TTLDuration time.Duration `mapstructure:"ttl-duration"`

// TTLNumBlocks, if non-zero, defines the maximum number of blocks a transaction
// can exist for in the mempool.
//
// Note, if TTLDuration is also defined, a transaction will be removed if it
// has existed in the mempool at least TTLNumBlocks number of blocks or if
// it's insertion time into the mempool is beyond TTLDuration.
TTLNumBlocks int64 `mapstructure:"ttl-num-blocks"`

Currently this config is local per validator. These values are the defaults:

ttl-duration = "1m15s"
ttl-num-blocks = 5 

Since block time is decreasing from 12 seconds to 6 seconds, the earliest something will be pruned from mempool is 5 * 6 seconds = 30 seconds. We may consider increasing ttl-num-blocks from 5 to 12 so that ttl-duration and ttl-num-blocks roughly matches.

Proposal

[in v3] update the default ttl-num-blocks to 12.

Evidence MaxAgeNumBlocks

consensus.evidence.MaxAgeDuration = 1814400000000000 (21 days)
consensus.evidence.MaxAgeNumBlocks = 120960

120960 blocks * 12 seconds per block = 1451520 seconds = 16.8 days
120960 blocks * 6 seconds per block = 725760 seconds = 8.4 days

we don't need to modify this parameter because evidence is only considered expired after 21 days and 120960 blocks (see here). Since 21 days remains intact, evidence won't be pruned prematurely.

Proposal

[not blocking v3] make the evidence params non governance modifiable and hard-code them to matching values ~21 days. See https://github.com/celestiaorg/CIPs/blob/main/cips/cip-16.md

x/ibc MaxExpectedTimePerBlock

// Params defines the set of Connection parameters.
type Params struct {
	// maximum expected time per block (in nanoseconds), used to enforce block delay. This parameter should reflect the
	// largest amount of time that the chain might reasonably take to produce the next block under normal operating
	// conditions. A safe choice is 3-5x the expected time per block.
	MaxExpectedTimePerBlock uint64 `protobuf:"varint,1,opt,name=max_expected_time_per_block,json=maxExpectedTimePerBlock,proto3" json:"max_expected_time_per_block,omitempty" yaml:"max_expected_time_per_block"`
}

Currently our MaxExpectedTimePerBlock is 5 * 15 seconds which exceeds the recommendation because our block time is actually ~11.77 seconds.

The parameter was added in cosmos/ibc-go#171. Related to cosmos/ibc#552. Reached out to IBC team about it.

Update: the IBC team informed us that the parameter is rarely used. On Mainnet Beta, all connections have a delay period of 0 which means the feature isn't used.

$ curl https://celestia-rest.publicnode.com/ibc/core/connection/v1/connections | grep delay_period
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 30318    0 30318    0     0   335k      0 --:--:-- --:--:-- --:--:--  336k
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"
      "delay_period": "0"

Proposal

Since the delay period feature isn't used on any active IBC connections on Mainnet Beta, I recommend we don't modify MaxExpectedTimePerBlock for now.

rootulp added a commit that referenced this issue Oct 15, 2024
Motivation in the **Mempool TTL duration** section of
#3959 (comment)

## Description

Modify the default consensus node config for mempools so that:

- TTLNumBlocks from 5 to 12 because 12 six second blocks = 72 seconds
which is comparable to the existing 75 second TTLDuration.
- ~~MaxTxBytes from approx 7.5 MiB to 8 MiB because it's easier to
understand and communicate~~
- ~~MaxTxsBytes from approx 37.6 MiB to 40 MiB because it's easier to
understand and communicate~~

## Testing

`make install && ./scripts/single-node.sh` then cat the config

```toml
version = "v1"
ttl-duration = "1m15s"
ttl-num-blocks = 12
```
@rootulp rootulp closed this as completed Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant