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

eth/catalyst: evict old payloads, type PayloadID #24236

Merged
merged 7 commits into from Jan 20, 2022

Conversation

protolambda
Copy link
Contributor

@protolambda protolambda commented Jan 13, 2022

Changes:

  • Remove PayloadResponse: dead code since last spec update, ForkchoiceUpdatedV1 returns the ID with status instead
  • Evict prepared payloads with a LRU of size 10. Preparing payloads with 10 different inputs in parallel, and reverting to the inputs of the first request should be very rare. This prevents the node from running out of memory over time when producing blocks. Update: LRU cache has been replaced with one specific for this functionality.
  • Type the PayloadID: cleans up API functions, no more hash / uint64 / hex conversions. And strict payload ID type parsing (previously > 8 bytes IDs were allowed)
  • Remove InvalidPayloadID: non-standard error
  • Changing to the thread-safe LRU prevents the ForkchoiceUpdatedV1 from panicking on concurrent map writes to the previous map

cc @MariusVanDerWijden

edit: updated, forgot goimports

Copy link
Member

@MariusVanDerWijden MariusVanDerWijden left a comment

LGTM, I've added a commit on top that enables some more tracing

eth/catalyst/api.go Outdated Show resolved Hide resolved
@protolambda
Copy link
Contributor Author

protolambda commented Jan 14, 2022

Amended commit to fix logging inconsistency: s/payloadid/payloadID/g

@MariusVanDerWijden
Copy link
Member

MariusVanDerWijden commented Jan 17, 2022

@protolambda I've added another test on top

eth/catalyst/api_test.go Outdated Show resolved Hide resolved
@MariusVanDerWijden
Copy link
Member

MariusVanDerWijden commented Jan 18, 2022

We discussed this PR in triage a bit and came to the conclusion that a cache is not a great structure for this.
It would be best to use a map and write our own eviction rules from the map, since we can be sure that they fit our use case then.
Do you want to do that @protolambda? If not, I can also take over

@protolambda
Copy link
Contributor Author

protolambda commented Jan 18, 2022

@MariusVanDerWijden Can you define the desired eviction rules? Simplicity is nice too. It's just temporary memory of the latest few payloads that produce/get functions need to exchange. I can help if you can describe what you need in more detail.

@@ -86,7 +88,8 @@ type ConsensusAPI struct {
eth *eth.Ethereum
les *les.LightEthereum
engine consensus.Engine // engine is the post-merge consensus engine, only for block creation
preparedBlocks map[uint64]*ExecutableDataV1
preparedBlocks *lru.Cache // preparedBlocks caches payloads (*ExecutableDataV1) by payload ID (PayloadID)
Copy link
Member

@karalabe karalabe Jan 18, 2022

Choose a reason for hiding this comment

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

This change is no really a good one. The role of a "cache" is to keep some recent data available-ish, but it's at the sole discretion of the implementation as to how long and when to evict. Whilst it might seem like not a bad idea at first to use a cache to store recent payloads, losing control of storage determinism can make thing in the future a lot harder to debug.

E.g. Certain caches have limits on the maximum size an item can have and will simply refuse to store something larger. In this context it could happen that if the payload exceeds a certain size, the cache would not store it anymore, making it unmineable. If all nodes would create a similarly large block they can't store, we could end u[ with a stalled network. Now I don't know if this particular cache behaves like that or not - or what the limits are - but this is exactly why a cache should not be used, because it's a disk access "optimization", not a storage data structure.

Copy link
Contributor Author

@protolambda protolambda Jan 18, 2022

Choose a reason for hiding this comment

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

Agreed about a cache not being the right fit here after a second look. But previously it would just keep growing indefinitely, and I discussed this change and eviction number with Marius before implementing it. Least-recently-used as eviction strategy + a fixed size limit seemed reasonable enough. Can you describe what you prefer instead?

Copy link
Member

@karalabe karalabe Jan 19, 2022

Choose a reason for hiding this comment

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

Depends how much work you want to put in and what the size of the set will be.

If we take the current cap of 10, then you could use a simple slice and just shift the contents on every insert and do a linear search on every retrieval. We're expected do do these ops rarely when it's our turn to sign, and even then once to create the payload and once to retrieve it within a 14 second time fame. A 10 item iteration doesn't matter in that scope.

If we were to make the set larger, the best solution would probably be a map as it was originally and an eviction order queue. That could be a channel or a circular queue (we have one in the code). But I don't see a reason to complicate it like that.

My 2c, unless there's a reason no to, just go with slice shifting / iteration.

eth/catalyst/api.go Outdated Show resolved Hide resolved
eth/catalyst/api.go Outdated Show resolved Hide resolved
eth/catalyst/api.go Outdated Show resolved Hide resolved
eth/catalyst/api.go Outdated Show resolved Hide resolved
)

const preparedPayloadsCacheSize = 10
Copy link
Member

@karalabe karalabe Jan 19, 2022

Choose a reason for hiding this comment

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

Pls delete this, it's dead code. Otherwise LGTM

@karalabe karalabe added this to the 1.10.16 milestone Jan 20, 2022
Copy link
Member

@karalabe karalabe left a comment

SGTM, will merge on green

@karalabe karalabe merged commit 514ae7c into ethereum:master Jan 20, 2022
1 check failed
sidhujag pushed a commit to syscoin/go-ethereum that referenced this pull request Jan 20, 2022
* eth/catalyst: evict old payloads, type PayloadID

* eth/catalyst: added tracing info to engine api

* eth/catalyst: add test for create payload timestamps

* catalyst: better logs

* eth/catalyst: computePayloadId return style

* catalyst: add queue for payloads

* eth/catalyst: nitpicks

Co-authored-by: Marius van der Wijden <m.vanderwijden@live.de>
Co-authored-by: Péter Szilágyi <peterke@gmail.com>
ValentinTrinque pushed a commit to vegaprotocol/go-ethereum that referenced this pull request Jan 25, 2022
* eth/catalyst: evict old payloads, type PayloadID

* eth/catalyst: added tracing info to engine api

* eth/catalyst: add test for create payload timestamps

* catalyst: better logs

* eth/catalyst: computePayloadId return style

* catalyst: add queue for payloads

* eth/catalyst: nitpicks

Co-authored-by: Marius van der Wijden <m.vanderwijden@live.de>
Co-authored-by: Péter Szilágyi <peterke@gmail.com>
MariusVanDerWijden added a commit to MariusVanDerWijden/go-ethereum that referenced this pull request Jan 27, 2022
* eth/catalyst: evict old payloads, type PayloadID

* eth/catalyst: added tracing info to engine api

* eth/catalyst: add test for create payload timestamps

* catalyst: better logs

* eth/catalyst: computePayloadId return style

* catalyst: add queue for payloads

* eth/catalyst: nitpicks

Co-authored-by: Marius van der Wijden <m.vanderwijden@live.de>
Co-authored-by: Péter Szilágyi <peterke@gmail.com>
JacekGlen pushed a commit to JacekGlen/go-ethereum that referenced this pull request May 26, 2022
* eth/catalyst: evict old payloads, type PayloadID

* eth/catalyst: added tracing info to engine api

* eth/catalyst: add test for create payload timestamps

* catalyst: better logs

* eth/catalyst: computePayloadId return style

* catalyst: add queue for payloads

* eth/catalyst: nitpicks

Co-authored-by: Marius van der Wijden <m.vanderwijden@live.de>
Co-authored-by: Péter Szilágyi <peterke@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants