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

If >33% nodes for any network desync, consensus will stall accordingly #6

Closed
kayabaNerve opened this issue May 10, 2022 · 3 comments
Closed
Labels
improvement This could be better
Milestone

Comments

@kayabaNerve
Copy link
Member

Blocks which fail contextual checks on consensus should still gleam the top block hash the other node is attempting to use, and after sufficient discrepancies being reported, drop that chain until resyncing occurs.

@kayabaNerve kayabaNerve added the bug Something isn't working label May 10, 2022
@kayabaNerve
Copy link
Member Author

The same block will also cause a desync if TXs are parsed distinctly, such as if our Monero lib is updated to expand a requirement and 34% of nodes update, using the expanded requirement to scan a TX the rest of the nodes won't acknowledge.

@kayabaNerve kayabaNerve added critical This is critical protocol labels Jun 5, 2022
@kayabaNerve kayabaNerve added this to the Testnet milestone Jul 20, 2022
@kayabaNerve
Copy link
Member Author

kayabaNerve commented Nov 15, 2022

Consensus won't actually stall completely. It'll solely fail to produce blocks for the 2/3rds of validators attempting to. This would also be extremely obvious. While this would be helpful to handle intelligently to be robust, I'm dropping "critical". It's also extremely difficult to trigger.

@kayabaNerve kayabaNerve added improvement This could be better and removed critical This is critical bug Something isn't working labels Nov 15, 2022
@kayabaNerve kayabaNerve removed this from the Protonet 0 milestone Nov 15, 2022
@TheArchitect108 TheArchitect108 added this to the Testnet milestone Nov 15, 2022
kayabaNerve pushed a commit that referenced this issue Feb 2, 2023
kayabaNerve added a commit that referenced this issue Mar 26, 2023
The original intent was to use inherent transactions to prevent needing to vote
on-chain, which would spam the chain with worthless votes. Inherent
transactions, and our Tendermint library, would use the BFT's processs voting
to also vote on all included transactions. This perfectly collapses integrity
voting creating *no additional on-chain costs*.

Unfortunately, this led to issues such as #6, along with questions of validator
scalability when all validators are expencted to participate in consensus (in
order to vote on if the included instructions are valid). This has been
summarized in #241.

With this change, we can remove Tendermint from Substrate. This greatly
decreases our complexity. While I'm unhappy with the amount of time spent on
it, just to reach this conclusion, thankfully tendermint-machine itself is
still usable for #163. This also has reached a tipping point recently as the
polkadot-v0.9.40 branch of substrate changed how syncing works, requiring
further changes to sc-tendermint. These have no value if we're just going to
get rid of it later, due to fundamental design issues, yet I would like to
keep Substrate updated.

This should be followed by moving back to GRANDPA, enabling closing most open
Tendermint issues.

Please note the current in-instructions-pallet does not actually verify the
included signature yet. It's marked TODO, despite this bing critical.
@kayabaNerve
Copy link
Member Author

Fixed in #241 and c182b80.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement This could be better
Projects
None yet
Development

No branches or pull requests

2 participants