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

Increase max_pov_size to 10MB #5334

Open
Tracked by #5444
sandreim opened this issue Aug 13, 2024 · 27 comments · May be fixed by #5884
Open
Tracked by #5444

Increase max_pov_size to 10MB #5334

sandreim opened this issue Aug 13, 2024 · 27 comments · May be fixed by #5884
Assignees

Comments

@sandreim
Copy link
Contributor

sandreim commented Aug 13, 2024

Currently we run relay chain with a max PoV size of 5MB on Polkadot/Kusama. We've recently discovered during benchmarking that the storage proof overhead increases significantly with the number of keys in storage such that the parachain throughput drops to 50% with 1 mil accounts for example.

Based on the numbers in #4399 we should be able to double the max pov size and still only require 50% of hw spec bandwidth at worst case.
I'd expect the CPU cost to increase to 100% at worst for the erasure encoding/decoding, we should determine it using subsystem benchmarks and see how it fits with the upcoming new hw specs.

CC @eskimor @burdges

@burdges
Copy link

burdges commented Aug 13, 2024

I do not have strong feelings on increasing PoV sizes near-term, but certianly if we put gluttons on kusama then it's nice to be able to pressure them properly.

We've recently discovered during benchmarking that the storage proof overhead increases significantly with the number of keys in storage such that the parachain throughput drops to 50% with 1 mil accounts for example.

Yes, we make storage proofs 4x larger than necessarily by inherreting the stupid radix 16 trie from ETH.

@s0me0ne-unkn0wn
Copy link
Contributor

So, a rough roadmap could be like that. In the next SDK release, we bump MAX_POV_SIZE to 10 Mb. Runtimes stay with their old 5 Mb limit compiled in, and they will not allow to build a block longer than 5 Mb. We're starting to prepare runtimes based on that new release while people upgrade nodes. When the supermajority upgrades, we're ready to enact new runtimes. After the runtimes are enacted, we get parablocks with 5 Mb proofs (10 Mb / 2) limited by collators (so all the 5 Mb may be used by user transactions without 25% reservation). Then, we can lift that halving limitation pointwise where we need it.

The other option is to make the proof size configurable, but I'm not sure how much sense it makes to do that, given that it's a complication anyway and the event of changing this constant is so rare.

CC @dmitry-markin any concerns from the networking layer side?

CC @eskimor @bkchr @skunert

Feel free to CC other relevant people.

@bkchr
Copy link
Member

bkchr commented Aug 21, 2024

Maximum code size is already controlled by the host configuration:

I strongly hope that we have not hard coded anywhere this 5MiB limit.

@s0me0ne-unkn0wn
Copy link
Contributor

I strongly hope that we have not hard coded anywhere this 5MiB limit.

As I see from the code, the MAX_POV_SIZE constant in primitives rules everything. Do you think bringing it to the configuration makes sense? I mean, if we want to change it every six months, then it makes sense definitely, but now we're seeing it as a big move and a one-shot change.

@sandreim
Copy link
Contributor Author

The MAX_POV_SIZE constant is also present in req/response protocols config.

I think we should remove all the constant references and use the configuration value.

In the future we might want to raise it above 10MB, I don't see any reason why not if the validators have enough bandwidth and CPU. We'd still be capped at 16MB on the parachain maximum response size of the block request protocol:

@bkchr
Copy link
Member

bkchr commented Aug 21, 2024

Do you think bringing it to the configuration makes sense?

Otherwise there is no consensus on this number. Which leads to the situation that we need to please all validators to upgrade. This is not really how a decentralized network should work. Using some upper number on the node makes sense, but it should give some leeway to the on chain value.

@alexggh
Copy link
Contributor

alexggh commented Aug 22, 2024

Do you think bringing it to the configuration makes sense?

Otherwise there is no consensus on this number. Which leads to the situation that we need to please all validators to upgrade. This is not really how a decentralized network should work. Using some upper number on the node makes sense, but it should give some leeway to the on chain value.

The runtime set code does something like this, so I guess that was the leeway which we now want to increase.

if self.max_pov_size > MAX_POV_SIZE {
     return Err(MaxPovSizeExceedHardLimit { max_pov_size: self.max_pov_size })
}

@eskimor eskimor mentioned this issue Aug 23, 2024
5 tasks
@eskimor
Copy link
Member

eskimor commented Aug 27, 2024

There are two values here:

  1. The limit on networking req/response protocols.
  2. The actual runtime configuration.

(1) poses an upper limit on (2). Yes ideally we would derive the network limit from the runtime configuration, but that would require some refactoring as we currently configure that value on node startup, when setting up the protocol.

It is worth double checking that (2) is correctly used everywhere in validation and not the constant. Other than that the process is as follows:

  1. Bump the networking limit ... this is backwards compatible and can be back ported.
  2. Once enough validators have upgraded we can change the runtime configuration.

Assuming we use the runtime configuration correctly (and persisted validation data, which should be derived from it), there can be no consensus issue. We nevertheless must have the majority of validators upgraded to the higher networking limit, otherwise honest nodes would not be able to fetch a large PoV, which could cause a finality stall.

@eskimor
Copy link
Member

eskimor commented Aug 27, 2024

Do you think bringing it to the configuration makes sense?

Otherwise there is no consensus on this number. Which leads to the situation that we need to please all validators to upgrade. This is not really how a decentralized network should work. Using some upper number on the node makes sense, but it should give some leeway to the on chain value.

The runtime set code does something like this, so I guess that was the leeway which we now want to increase.

if self.max_pov_size > MAX_POV_SIZE {
     return Err(MaxPovSizeExceedHardLimit { max_pov_size: self.max_pov_size })
}

Indeed. First the node side limit, then the runtime value.

@dmitry-markin
Copy link
Contributor

One thing regarding the networking req/resp protocols limit to keep in mind, is that the block response limit was set for minimal supported network bandwidth. I.e., it should not time out even on the slowest supported connections.

I don't have the numbers at hand, but the 16 MB block response should not time out in 20 seconds, so the minimum bandwidth is presumably 1 MB/sec in the spec. If we raise this limit, we should also make sure we either increase the request-response protocol timeout (but this can introduce more latency with unresponsive peers) or raise the bandwidth requirements.

@alexggh
Copy link
Contributor

alexggh commented Aug 27, 2024

From: https://wiki.polkadot.network/docs/maintain-guides-how-to-validate-polkadot#reference-hardware

The minimum symmetric networking speed is set to 500 Mbit/s (= 62.5 MB/s). This is required to support a large number of parachains and allow for proper congestion control in busy network situations.

So at least from theoretical point of view we should have bandwidth for validators.

@dmitry-markin
Copy link
Contributor

From: https://wiki.polkadot.network/docs/maintain-guides-how-to-validate-polkadot#reference-hardware

The minimum symmetric networking speed is set to 500 Mbit/s (= 62.5 MB/s). This is required to support a large number of parachains and allow for proper congestion control in busy network situations.

So at least from theoretical point of view we should have bandwidth for validators.

Also, regular full nodes must be able to follow the chain.

@aaron2048
Copy link

+1 for this. Moonbeam has adopted Asynchronous backing and increased the EVM gas per block and we're running into difficulties due to this limitation.

@AndreiEres
Copy link
Contributor

Moving comments from #5753

We should consider networking speed limitations if we want to increase the maximum PoV size to 10 MB. The current PoV request timeout is set to 1.2s to handle 5 consecutive requests during a 6s block. With the number of parallel requests set to 10, validators will need the following networking speeds:

  • 5 MB PoV: at least 42 MB/s, ideally 50 MB/s.
  • 10 MB PoV: at least 84 MB/s, ideally 100 MB/s.

The current required speed of 50 MB/s aligns with the 62.5 MB/s specified in the reference hardware requirements. Increasing the PoV size to 10 MB may require a higher networking speed. This is worst case scenario, when all blocks you need to recover are full.

@sandreim
Copy link
Contributor Author

sandreim commented Sep 19, 2024

Moving comments from #5753

We should consider networking speed limitations if we want to increase the maximum PoV size to 10 MB. The current PoV request timeout is set to 1.2s to handle 5 consecutive requests during a 6s block.

With the number of parallel requests set to 10, validators will need the following networking speeds:
Where does this number come from ?

  • 5 MB PoV: at least 42 MB/s, ideally 50 MB/s.

If we have around 6-7 approvals per validator, then we'd need to recover 5 * 7 = 35 MB per relay chain block, that is almost 6MB/s required to keep up with finality.

For backing, assuming there async backing limits (max_candidate_depth 3, max ancestry len 2), there should be at most 5 * 3 * 2 = 5 MB/s

Which means a total of lets say 11MB/s for all PoV fetches.

  • 10 MB PoV: at least 84 MB/s, ideally 100 MB/s.

This would mean 22MB/s based on the above math which is well under the spec leaving room for all other gossip and relay chain block fetching.

@burdges
Copy link

burdges commented Sep 19, 2024

We think 100MB/s = 1Gb/s has good odds of handling 15 approvals per valudators then? Or do we think there needs to be a lot more slack somehow?

@sandreim
Copy link
Contributor Author

We think 100MB/s = 1Gb/s has good odds of handling 15 approvals per valudators then? Or do we think there needs to be a lot more slack somehow?

Networking should probably be enough even now for 15. I'd be worried about CPU usage, you'd have 15 * 2 = 30s of execution every 6s seconds on top of backing duties. With updated hw specs means 5cores are busy leaving 3 more for relay chain, networking and parachain consensus. Out of those 3 cores, we'd be easily using one just for erasure coding.

@albertov19
Copy link

albertov19 commented Sep 24, 2024

Hey, are there timelines for when we might bump the PoV to 10 MB?

One of the current issues is that with Async Backing, we bumped the execution time to 2 seconds, but the PoV was kept the same, so we are not reaping the full benefits of increasing the execution time from 0.5 to 2 seconds.

@sandreim
Copy link
Contributor Author

sandreim commented Sep 26, 2024

The plan is to get it done this year. In terms of code changes this is very little work, but requires significant testing before we go to Polkadot.

@s0me0ne-unkn0wn
Copy link
Contributor

@bkchr After some research, this is what the situation looks like.

I was pleased to discover that max_pov_size is already part of both the host configuration and the persisted validation data. However, this value is never used in runtime (though the original issue paritytech/polkadot#1572 presumed such a usage). It is used in the candidate validation subsys to check the PoV is not too large before performing the actual validation, and also it is used in the collator code to prevent collator from building over this limit (but the check in the collator obviously ignores per dispatch class limits).

In the runtime, the hardcoded MAX_POV_SIZE constant is always used. I believe this is due to the chicken-and-egg situation: both persisted validation data and abridged host configuration data, from where the limit may be derived, are set by the set_validation_data inherent, but by the time of executing the inherents we must already have block length and PoV size limits in place.

One thing that could be done is to set MAX_POV_SIZE constant to the maximum possible value and use it as a bootstrap value when starting to build a parablock, and then, when set_validation_data inherent arrives, overwrite the limits with new ones. The implications are frame_system::BlockWeights and BlockLength couldn't be pallet::constants anymore as we'd have to change them in runtime.

Are you okay with such an approach? Or do you have any better ideas?

@bkchr
Copy link
Member

bkchr commented Sep 28, 2024

Yeah the situation not being the best. I would assume that at least the runtime side on the relay chain is using the value from the HostConfiguration and at best also the backing etc subsystems use the variable pov size. So, this value can be changed without requiring coordinated upgrades of all validators etc.

For the parachain runtime, using a const sounds sensible. This value doesn't change that often and the assumption that it only goes up is also fine to assume. So, the only downside would be that a parachain may not be able to use the full pov size until they have done a runtime upgrade. However, I think this is reasonable.

@s0me0ne-unkn0wn
Copy link
Contributor

I would assume that at least the runtime side on the relay chain is using the value from the HostConfiguration

I'm not sure I'm following your thoughts here. When a validator needs to validate a parablock, be it backing or approvals or whatever, it comes through the candidate validation subsystem, which, before instantiating the PVF, surely checks that the PoV is not larger then the limit noted in the persistent validation data, but that's an offchain check.

Still, we need to change MAX_POV_SIZE constant and perform a relay chain runtime upgrade before we can increase the limit because the host configuration pallet will not allow us to go over that hardcoded limit. With this limit being a Polkadot primitive, I'm not sure if we need an RFC or something to bump it.

@bkchr
Copy link
Member

bkchr commented Sep 30, 2024

I'm not sure I'm following your thoughts here. When a validator needs to validate a parablock, be it backing or approvals or whatever, it comes through the candidate validation subsystem, which, before instantiating the PVF, surely checks that the PoV is not larger then the limit noted in the persistent validation data, but that's an offchain check.

Perfect! That is what I meant! max_pov_size is fetched from the on chain state.

Still, we need to change MAX_POV_SIZE constant and perform a relay chain runtime upgrade before we can increase the limit because the host configuration pallet will not allow us to go over that hardcoded limit.

That the hardcoded limit is used there again, is IMO not correct. The max_pov_size should be controlled by the HostConfiguration and not limited by some "random" constant.

With this limit being a Polkadot primitive, I'm not sure if we need an RFC or something to bump it.

I would argue that we don't need any RFC. Or better, depends a little bit if the maximum request/response size for the individual protocols is specced. In a perfect world I would assume that these values are not specced.

@s0me0ne-unkn0wn s0me0ne-unkn0wn linked a pull request Oct 1, 2024 that will close this issue
github-merge-queue bot pushed a commit that referenced this issue Oct 2, 2024
Quoting @bkchr (from
[here](#5334 (comment))):

> That the hardcoded limit is used there again, is IMO not correct. The
`max_pov_size` should be controlled by the `HostConfiguration` and not
limited by some "random" constant.

This PR aims to change the hard limit to a not-so-random constant,
allowing more room for maneuvering in the future.
@aaron2048
Copy link

The plan is to get it done this year. In terms of code changes this is very little work, but requires significant testing before we go to Polkadot.

Does that mean target is to have it live on Polkadot by year end? Since fully adopting asynchronous backing on Moonbeam, we expanded from 15M gas per block to 60M but since the POV is the same, cost for some tx that are bound by proof size have quadrupled so we're hoping to get this change as soon as possible.

@sandreim
Copy link
Contributor Author

sandreim commented Oct 2, 2024

Does that mean target is to have it live on Polkadot by year end?

yes

Since fully adopting asynchronous backing on Moonbeam, we expanded from 15M gas per block to 60M but since the POV is the same, cost for some tx that are bound by proof size have quadrupled so we're hoping to get this change as soon as possible.

I was looking at some metrics for Moonbeam proof size and it doesn't seem to be a bottleneck, see below (source https://www.polkadot-weigher.com/history)

Screenshot 2024-10-02 at 23 51 03

@albertov19
Copy link

@sandreim, thanks for providing this. I'll forward it internally. However, the issue is that if there is a block with 60M gas of PoV-heavy transactions, you might see these numbers go up to 100%.

Our blocks were 15M Gas with a 5 MB PoV. Due to the increased execution time we bumped it to 60M gas but for the same 5 MB PoV. Hence, we had to penalize PoV-heavy Ethereum transactions from a gas perspective by bumping the gas estimation 4x. Only those transactions are affected. We estimate gas for execution, storage growth, and PoV and use the estimation or the worst-case scenario.

@bkchr
Copy link
Member

bkchr commented Oct 9, 2024

@albertov19 I still think that you should build a reclaim like functionality that also pays back fees.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Backlog
Development

Successfully merging a pull request may close this issue.

10 participants