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

stateDerivedFromBlock.lastSlotReceipts uses unreasonable too much RAM #108

Open
wqking opened this issue Oct 30, 2019 · 12 comments
Open

stateDerivedFromBlock.lastSlotReceipts uses unreasonable too much RAM #108

wqking opened this issue Oct 30, 2019 · 12 comments
Assignees
Labels
enhancement New feature or request

Comments

@wqking
Copy link
Contributor

wqking commented Oct 30, 2019

In a simple test (not many slots, maybe just one or two days), a lastSlotReceipts can hold more than 5M receipts, which is more than 100M RAM. And if there are more slots, I ever observed it uses too much RAM and crashes the OS.

I'm not sure if it can be eliminated, if not, we may need to use external database for it instead of residenting in memory. If we are going to use database, I can do it.
For the choice of database, current database for the chain uses very much disk. I think SQLite maybe better. The disadvantage of using SQLite is that we introduce a new kind of database to Synapse.

@wqking wqking added the enhancement New feature or request label Oct 30, 2019
@meyer9
Copy link
Member

meyer9 commented Oct 30, 2019

It shouldn't ever need to calculate state that far in the future. Where is this being called from? Maybe we should set some limits on the derived state (5 epochs or so).

@wqking
Copy link
Contributor Author

wqking commented Oct 30, 2019

StateManager.GetStateForHashAtSlot calls stateDerivedFromBlock.deriveState which populates the receipts array. GetStateForHashAtSlot is called when adding a block to the statemap (AddBlockToStateMap).

@meyer9
Copy link
Member

meyer9 commented Oct 30, 2019

So I guess we should reject blocks too far from the current tip so we don't DoS the node when trying to calculate the state at that time?

@wqking
Copy link
Contributor Author

wqking commented Nov 5, 2019

Currently the Receipt list are only used for debug log and SyncManager.postProcessHook, so maybe,
1, if the receipts are only used for that purpose, we may remove them from memory after they are used by the hook.
2, if the receipts are used for other purpose, we may store it to local database, then load them on demand.
3, or as you said, we may reduce the generating of the receipts.

@meyer9
Copy link
Member

meyer9 commented Nov 5, 2019

Let's reduce receipt generation. In ProcessBlock, ensure the block being processed is within 3 epochs of the parent block. In GetEpochInformation, make sure we're not getting a block more than 3 epochs from the current tip slot.

@wqking
Copy link
Contributor Author

wqking commented Nov 5, 2019

OK, I will do it.

@wqking wqking self-assigned this Nov 5, 2019
@wqking
Copy link
Contributor Author

wqking commented Nov 6, 2019

After some research, I doubt limiting block age can work, because no matter the blocks are old or new, they will be processed eventually.

What I've found is, after Beacon is shut down for some time, and after it runs again, the function primitives.State.ProcessSlots will try to make the slot catching up to latest slot, so it calls ProcessEpochTransition many times on each epoch, which will generate huge amount of receipts.
That also means the longer the Beacon was shut down, the more receipts it will generate after run.
I think we may need to limit the number of slots (or epoches) that ProcessSlots can process to generate the receipts?

Another problem (maybe not a problem, just how it should work) is, there are 256 validators in the .json config, but I only run one validator. Beacon still considers all 256 validators are 'Active' and generate receipts for them. I guess it's how Beacon works, it requires the validators running, otherwise give penalty to them?

@meyer9
Copy link
Member

meyer9 commented Nov 6, 2019

It does that when the validation needs them to create new blocks. That's why limiting the age will work because the validator will stop until we sync to a block closer to the current time.

@wqking
Copy link
Contributor Author

wqking commented Nov 7, 2019

I tried a simple fix here on 7d1b61d
Please reivew it.
I didn't observe those limitations were triggered so the memory usage is not decreased.
Also, when Beacon starts up, when it loads blocks from database and populates the state map, it will generate large amount of receipts.

@meyer9
Copy link
Member

meyer9 commented Nov 7, 2019

Where is it calling derive state from that generates a ton of receipts? It shouldn't have to derive any state when loading. It will have to derive state if a validator connects and requests epoch info after being offline for a while.

I did notice you have the wrong inequality on GetEpochInformation

@wqking
Copy link
Contributor Author

wqking commented Nov 7, 2019

The function populateStateMap in file beacon.database calls Blockchain.AddBlockToStateMap, which will populate the receipts. That's from my memory, I will test to check it.

@meyer9
Copy link
Member

meyer9 commented Nov 7, 2019

That should only need to derive state between blocks I think though.

If Block A has slot 10 and block B has slot 20 and A is B's parent, it should derive state between 10 and 20 which makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants