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

all: separate catalyst package #24280

Merged
merged 6 commits into from Jan 31, 2022
Merged

Conversation

rjl493456442
Copy link
Member

@rjl493456442 rjl493456442 commented Jan 24, 2022

This PR separate the mega catalyst package into two small one eth/catalyst and les/catalyst in sake of simplification.

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

MariusVanDerWijden commented Jan 24, 2022

@rjl493456442 I've added a commit on top that adds some doc and moves some methods around. If you agree with the docs, we should also add them to the LES package (didn't do that yet, since I wanted to have someone look over it before doing so)

}
td := api.eth.BlockChain().GetTd(newHeadBlock.Hash(), newHeadBlock.NumberU64())
if td != nil && td.Cmp(api.eth.BlockChain().Config().TerminalTotalDifficulty) < 0 {
return &InvalidTB
return &beacon.InvalidTB
Copy link
Member Author

@rjl493456442 rjl493456442 Jan 25, 2022

Choose a reason for hiding this comment

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

@MariusVanDerWijden

What if the td is nil while the headBlock is present? In case of some database corruption?

Copy link
Member

@MariusVanDerWijden MariusVanDerWijden Jan 25, 2022

Choose a reason for hiding this comment

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

This should only happen if the database is fried and we have the headBlock in DB, but not the TD for it in the DB.
I'm not sure if we should handle it here or if this should blow up at some other point.
Maybe @karalabe can weigh in here: Should we double check and error or should we ignore and eventually blow up in case of database corruption

Copy link
Member Author

@rjl493456442 rjl493456442 Jan 25, 2022

Choose a reason for hiding this comment

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

What about returning a &beacon.GenericServerError here?

Copy link
Member

@MariusVanDerWijden MariusVanDerWijden Jan 25, 2022

Choose a reason for hiding this comment

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

Yeah, might be better than silently accepting

@rjl493456442
Copy link
Member Author

rjl493456442 commented Jan 25, 2022

@MariusVanDerWijden I applied your changes to LES and also moved some common functions into the beacon package as well, please take another look.

Also I left another comment, please check it.

Copy link
Member

@MariusVanDerWijden MariusVanDerWijden left a comment

LGTM

@karalabe karalabe added this to the 1.10.16 milestone Jan 31, 2022
@karalabe karalabe merged commit 9da25c5 into ethereum:master Jan 31, 2022
1 check was pending
sidhujag pushed a commit to syscoin/go-ethereum that referenced this issue Feb 1, 2022
* all: seperate catalyst package

* eth/catalyst: moved some methods, added docs

* eth/catalyst, les/catalyst: add method docs

* core, eth, les, miner: move common function to beacon package

* eth/catalyst: goimported

* cmd/utils, miner/stress/beacon: naming 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 issue May 26, 2022
* all: seperate catalyst package

* eth/catalyst: moved some methods, added docs

* eth/catalyst, les/catalyst: add method docs

* core, eth, les, miner: move common function to beacon package

* eth/catalyst: goimported

* cmd/utils, miner/stress/beacon: naming 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