near / nearcore

Reference client for NEAR Protocol
https://near.org
GNU General Public License v3.0
2.31k stars 619 forks source link

Add a way to artificially increase the size of ChunkStateWitness #11687

Closed jancionear closed 3 months ago

jancionear commented 3 months ago

This PR adds a new feature to neard: artificial_witness_size. When enabled, the node will read artificial_witness_size_to_add from config.json and add this many bytes of data to all produced witnesses. Data is added by injecting a dummy transaction with random contract code of desired size. The injected transaction is automatically removed before witness validation.

artificial_witness_size_to_add is a MutableConfigValue so it can be adjusted on the fly without restarting the node.

The feature has to be enabled on all nodes in the network because normal chunk validators will reject witnesses with the injected transaction when the feature isn't enabled.

Doing it by injecting transactions doesn't change the serialization format, so it's compatible with existing networks.

The extra transaction is only added when artificial_witness_size_to_add is larger than zero, so it's safe to go back to using binaries where this feature is disabled by setting artificial_witness_size_to_add to zero and then disabling the feature.

We can use it to test how witness distribution behaves at larger sizes and to simulate doomsday scenarios.

I manually tested that the feature works on localnet by setting up a 4 node/6 shard localnet and forcing it to send 100 MB witnesses. The network struggled, but it went forward at 0.1 bps.

I increased the number of buckets for witness size because the current ones go only up to 28 MB. After the change they go up to 120 MB.

Refs: https://github.com/near/nearcore/issues/11184

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 80.76923% with 5 lines in your changes missing coverage. Please review.

Project coverage is 71.73%. Comparing base (89fad46) to head (d5cb7d9).

Files Patch % Lines
chain/client/src/client.rs 0.00% 4 Missing :warning:
nearcore/src/dyn_config.rs 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #11687 +/- ## ========================================== - Coverage 71.74% 71.73% -0.01% ========================================== Files 790 790 Lines 161838 161862 +24 Branches 161838 161862 +24 ========================================== + Hits 116106 116111 +5 - Misses 40689 40711 +22 + Partials 5043 5040 -3 ``` | [Flag](https://app.codecov.io/gh/near/nearcore/pull/11687/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | Coverage Δ | | |---|---|---| | [backward-compatibility](https://app.codecov.io/gh/near/nearcore/pull/11687/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `0.23% <0.00%> (-0.01%)` | :arrow_down: | | [db-migration](https://app.codecov.io/gh/near/nearcore/pull/11687/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `0.23% <0.00%> (-0.01%)` | :arrow_down: | | [genesis-check](https://app.codecov.io/gh/near/nearcore/pull/11687/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `1.35% <3.84%> (+<0.01%)` | :arrow_up: | | [integration-tests](https://app.codecov.io/gh/near/nearcore/pull/11687/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `37.80% <80.76%> (-0.01%)` | :arrow_down: | | [linux](https://app.codecov.io/gh/near/nearcore/pull/11687/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `69.13% <34.61%> (-0.03%)` | :arrow_down: | | [linux-nightly](https://app.codecov.io/gh/near/nearcore/pull/11687/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `71.23% <80.76%> (-0.01%)` | :arrow_down: | | [macos](https://app.codecov.io/gh/near/nearcore/pull/11687/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `51.39% <34.61%> (-1.22%)` | :arrow_down: | | [pytests](https://app.codecov.io/gh/near/nearcore/pull/11687/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `1.58% <3.84%> (+<0.01%)` | :arrow_up: | | [sanity-checks](https://app.codecov.io/gh/near/nearcore/pull/11687/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `1.38% <3.84%> (+<0.01%)` | :arrow_up: | | [unittests](https://app.codecov.io/gh/near/nearcore/pull/11687/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `66.33% <80.76%> (+<0.01%)` | :arrow_up: | | [upgradability](https://app.codecov.io/gh/near/nearcore/pull/11687/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `0.28% <0.00%> (-0.01%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jancionear commented 3 months ago

This seems alright, though to me it seems like appending a field, or perhaps otherwise making the witness serialization invalid from the first principles while this feature is enabled would be a safer bet. But I'm not quite sure why it is necessary to have this be compatible with existing networks...

(Making it invalid to deserialize normally might be just length * 1024 || random_data || actual_witness_data or something)

Setting up a new network is a lot of work, so I wanted to make it compatible with existing networks (forknet-20, forknet-100, statelessnet). Changing the serialization format would require us to restart all of the nodes in a network at once, which would be distruptive.

I'm thinking about running a witness size test on statelessnet, as it's the most "real" and diverse test environment we have, and it wouldn't be possible to do a coordinated restart there.

jancionear commented 3 months ago

Closing in favor of https://github.com/near/nearcore/pull/11703