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

WIP: allow totalDifficulty to be the same #145

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ligi
Copy link

@ligi ligi commented Jan 20, 2022

this is a PR to discuss and progress to fix #132
as a WIP/Draft currently as I am not sure about is the impact on non ethereum chains - for our Ethereum case it seems to work fine cc @skylenet

PS: maybe also one refactor would be nice - renaming difficulty to totalDifficulty - this really send me on the wrong track that the difficulty I saw not moving anymore and not being 0 was actually the totalDifficutly just named difficulty.

@CLAassistant
Copy link

CLAassistant commented Jan 20, 2022

CLA assistant check
All committers have signed the CLA.

@splix
Copy link
Member

splix commented Jan 20, 2022

@ligi thank you.

It should certainly work for some test network and similar, but I don't think it's a good idea for others.
For most of others it would be simply sub-optimal because now each block would be processed multiple times. Without such filter, each time a same block became available on each of upstreams it would be considered as a new block by Dshackle. It's not a big issue, but it makes a lot of unnecessary requests, unnecessary cache refresh, etc.

Technically it's possible to make it as an option that would disable block validation only for some networks. That would be a better solution.

But what I'm worried most is that it contradicts the architecture of the Dshackle. It's based on the idea that it needs to distinguish bad upstreams from good upstreams. And totalDifficulty is a critical element of that. If Dshackle cannot do that check then many other assumptions of the current state of the upstreams may go wrong. And honestly, it doesn't make any sense to use Dshackle in this case, it's going to be no better than Nginx, Envoy and other protocol agnostic load balancers.

So, if you want the ability to disable the totalDifficulty check, please make it as a configurable option. I can approve that. Though I cannot guarantee that Dshackle would always work in such situations.

@ligi
Copy link
Author

ligi commented Jan 21, 2022

@splix thanks for the answer!

It should certainly work for some test network and similar, but I don't think it's a good idea for others.

The difficulty will 0 and total difficulty will increase no more on Ethereum main-net after "the merge" - so very likely this year. Also I guess most Ethereum networks will behave this way over time.
So I think for all Ethereum networks it is best to just use the block-height and not the difficulty to see progress. Currently I see no advantage in using the difficulty over the block-height. The only place I can imagine it has some influence is a non Ethereum chain. Not sure if a config option is the right choice or to make it a property of Ethereum in the system.

And honestly, it doesn't make any sense to use Dshackle in this case, it's going to be no better than Nginx, Envoy and other protocol agnostic load balancers.

Ethereum DevOps was using Dshackle for a bit as a BlockChain aware LoadBalancer is nice in some cases. E.g. for the check if a chain has peers, is syncing, is at the same height as the others. But maybe it is time to create something Ethereum RPC native instead of BlockChain native - would be much simpler as far as I see.

@skylenet
Copy link
Contributor

But what I'm worried most is that it contradicts the architecture of the Dshackle. It's based on the idea that it needs to distinguish bad upstreams from good upstreams. And totalDifficulty is a critical element of that. If Dshackle cannot do that check then many other assumptions of the current state of the upstreams may go wrong. And honestly, it doesn't make any sense to use Dshackle in this case, it's going to be no better than Nginx, Envoy and other protocol agnostic load balancers.

@splix Dshackle seems to be supporting Ethereum now for a long time. Myself and many other have been using it in the Ethereum ecosystem. I think it's worth to think about a solution on how Dshackle could be also used for post merge from PoW to PoS. In theory totalDifficulty could be replaced by something else in PoS based Ethereum chains.

But maybe it is time to create something Ethereum RPC native instead of BlockChain native - would be much simpler as far as I see.

I think that @splix has done a great job with Dshackle and I would prefer to support existing OpenSource projects that have been around and used by people (which is the case). So, I think it's worth it to not reinvent the wheel, or "make a simpler wheel", but instead make Dshakle work with PoS based Ethereum. We would all benefit from it! :)

@splix
Copy link
Member

splix commented Feb 4, 2022

@ligi well, I hear about Ethereum dropping PoW since 2016, so I guess it's safe to assume that we need to support current design for at least 5 more years :)

Anyway, Dshackle must have a way to distinguish good and bad node when they forked to different chains, and that could happen regardless a consensus algorithm. With PoW it's absolutely normal situation and happens every few minutes, so at least for now it's important to handle it. For PoS it could be a different field in a JSON, not necessary called totalDifficulty, but some number helping to chose a right side. Block Height is not that number, because on both forks it would be same number, but only one of the forks is valid.

I understand though, that for test networks that field may be missing which prevents from testing with Dshackle. So for this case it could be useful to disable difficulty check and use just height, but it must be optional and configurable.

@MysticRyuujin
Copy link
Contributor

MysticRyuujin commented Feb 15, 2022

Isn't this one of the issues with why Fantom doesn't work also? I think making it a chain option should suffice no? I don't think this needs to turn into a PoW vs PoS argument 🙂

I'd also like to see xDai / Gnosis chain support and it'll probably be the same problem over there too wont it?

vdoflip pushed a commit to jit-strategies/dshackle that referenced this pull request Aug 1, 2023
- move noisy logging to trace level
- log entire broken ws message
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.

Issues with running on the ethereum merge testnets
5 participants