perf: De-duplicate writes in SaveABCIResponses / SaveFinalizeBlockResponse #1976
Comments
8000
ValarDragon
added
enhancement
New feature or request
needs-triage
This issue/PR has not yet been triaged by the team.
labels
Jan 6, 2024
|
will give it a go |
faddat
added a commit
to faddat/cometbft
that referenced
this issue
Jan 6, 2024
|
Thanks @ValarDragon ❤️ for putting in the effort. |
melekes
added a commit
that referenced
this issue
Jan 11, 2024
for performance reasons. Closes #1976
3 tasks
melekes
added a commit
that referenced
this issue
Jan 29, 2024
for performance reasons. Closes #1976
github-merge-queue bot
pushed a commit
that referenced
this issue
Feb 21, 2024
for performance reasons. Closes #1976 --- #### PR checklist - [ ] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] Updated relevant documentation (`docs/` or `spec/`) and code comments
mergify bot
pushed a commit
that referenced
this issue
Feb 21, 2024
for performance reasons. Closes #1976 --- #### PR checklist - [ ] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] Updated relevant documentation (`docs/` or `spec/`) and code comments (cherry picked from commit f2c7466) # Conflicts: # internal/state/store.go
mergify bot
pushed a commit
that referenced
this issue
Feb 21, 2024
for performance reasons. Closes #1976 --- #### PR checklist - [ ] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] Updated relevant documentation (`docs/` or `spec/`) and code comments (cherry picked from commit f2c7466) # Conflicts: # state/store.go # state/store_test.go
greg-szabo
pushed a commit
that referenced
this issue
Feb 22, 2024
for performance reasons. Closes #1976 --- #### PR checklist - [ ] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] Updated relevant documentation (`docs/` or `spec/`) and code comments
melekes
added a commit
that referenced
this issue
Feb 22, 2024
for performance reasons. Closes #1976 --- #### PR checklist - [ ] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] Updated relevant documentation (`docs/` or `spec/`) and code comments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Feature Request
SaveABCIResponses is called on every block during consensus execution + block sync, and is blocking consensus from proceeding.
We see that for nodes which want to serve the
block_resultsquery, we pay a 2x write amplification over the data we want to serve.The 2x write amplification in SaveABCIResponses can get extremely expensive for nodes, when there are many events. This occurs during a current problematic mainnet scenario detailed below, that currently affects all cosmos chains. Would be great if we can get a reduction at the comet layer soon for its part of the slowdown in this process!
Summary
Over a 100 block sync, where only a fraction of blocks have spam txs as detailed below, we see SaveABCIResponses take a large amount of time. (10 seconds! The state machine exec + commit time for this is 55 seconds. For a more "normal" 100 block range, we see this take .23 seconds)
Problem Definition / Context
When people send IBC packets with large amounts of data (400kb), nodes begin falling behind. (This is the "bananaking" attacks)
The biggest culprit of problems for nodes falling behind comes from SaveABCIResponse where we suffer a 2x write overhead: https://github.com/cometbft/cometbft/blob/main/internal/state/store.go#L561-L601 . We see nodes having to write an extra 100mb/s when these txs come in. We also see 10+ of these getting into a block. (Which matches the expected 100MB/s overhead being measured)
During this time, we observe all RPC nodes have a very hard time keeping up, network block times slow down, and validators who do not run
DiscardABCIResponsesmiss blocks.There are other issues that go into having problems here, but this particular comet location definitely seems like a strong mitigation here going forward. The situation is that this msg, if
xbytes large, has ABCIResponses of size13xlarger thanx. This lead to a26xwrite overhead in Comet here. CPU overheads in the SDK + IBC + comet call stack are present for this pattern. The gas per byte is already underpriced, but this makes it more costly. CometBFT block gossip also gets quite expensive on big blocks. (Talking to other teams to reduce the ABCI response size overhead + CPU times)Without knowing this can become a system bottleneck, the reason for the 2x write overhead is quite reasonable imo!
Proposal
Notice that in https://github.com/cometbft/cometbft/blob/main/internal/state/store.go#L561-L601 we are setting the same data twice.
First we set at key
block_results/{HEIGHT}value{data}. Then we SetSync at keylatest_block_resultsvalue{data, height}, and we overwrite the latter every block.I think we should instead be able to, at better performance, just only set
SetSyncat keyblock_results/{HEIGHT}, and then:block_results/{HEIGHT - 1}Please let me know thoughts! I think this would really alleviate this attack vector thats being demonstrated in prod right now.
The text was updated successfully, but these errors were encountered: