QGB ADR-005 Reducing QGB module state usage #657

rach-id · 2022-08-24T12:48:29Z

Closing #656

rootulp

Great work articulating the problem! Most of my comments are nits. I'm super curious on what the "QGB Rollup" design entails but it doesn't seem necessary to flesh out now.

docs/architecture/ADR-005-QGB_posting_confirms_in_reserved_namespace.md

rootulp · 2022-08-25T01:46:37Z

docs/architecture/ADR-005-QGB_posting_confirms_in_reserved_namespace.md

+### Keep the existing design
+
+Keeping the current design would entail using the state extensively.
+This proves bad when the state grows after a few hundred attestations, and performing checks on the `Valset Confirms` and `DataCommitment Confirms` becomes so expensive.


[nit]

Suggested change

This proves bad when the state grows after a few hundred attestations, and performing checks on the `Valset Confirms` and `DataCommitment Confirms` becomes so expensive.

This proves bad when the state grows after a few hundred attestations, and performing checks on the `Valset Confirms` and `DataCommitment Confirms` becomes too expensive.

I remember hearing that this state is bounded by some fixed upper limit. Is it worth explaining that limit here?

check now please, added a sentence explaining that.

docs/architecture/ADR-005-QGB_posting_confirms_in_reserved_namespace.md

rootulp · 2022-08-25T02:05:32Z

docs/architecture/ADR-005-QGB_posting_confirms_in_reserved_namespace.md

+
+### Negative
+
+- Reducing the checks applied on the confirms: everyone will be able to post commitments, and we will have no way to sanitize them before adding them to a block. This would add an extra overhead for the relayer to choose the right commitments. However, it is alright, since we will not have a lot of Also, we will need to decide if this will be relayers, and we can give them enough computation power to compensate the overhead.


However, it is alright, since we will not have a lot

the rest of this sentence got lost

check now please

docs/architecture/ADR-005-QGB_posting_confirms_in_reserved_namespace.md

…espace.md Co-authored-by: Rootul Patel <rootulp@gmail.com>

Co-authored-by: Rootul Patel <rootulp@gmail.com>

rootulp

Thanks for addressing all my nits! I still don't understand what data needs to be published to a reserved namespace vs. non-reserved namespace but it sounds like you're working on a proposal for that so will re-review after its published

rootulp · 2022-08-25T16:39:59Z

docs/architecture/ADR-005-QGB_posting_confirms_in_reserved_namespace.md

+
+## Context
+
+Currently, the QGB is using the state to save all the attestations and confirms it needs.


the state

[optional] it may help non-familiar readers (i.e. me) to clarify what this state is. Is it specifically state stored within the QGB module which would end up bloating the celestia-app state?

check now please?

evan-forbes

just to note some things @sweexordious and I discussed synchronously:

For any new more stateless approach, we need to first have a plausible-potentially-hand-wavy form of slashing. As already mentioned, deciding on when we actually want this formal mechanism for slashing is also important.
A more stateless approach could be as simple as keeping identical transaction types, but not storing the confirmations in the state! We'd still have the valset/data commitment requests be handled and stored by the state machine, and the mapping between validators and their orchestrator keys, but the rest would simply be charged gas fees and be stored in block data. Relayers and orchestrators can then reconstruct a local copy of the state using the block data.
One hand-wavy-potentially-viable formal slashing mechanism could involve some sort of commit-reveal scheme, where anyone can post a bond and claim that a validator should be slashed for a specific violation. It is then up to any other honest participant to refute that claim. If successful, then they get the bond. This would likely require other additional data be kept in state, but that's still to be investigated. As mentioned, it might be easier to implement fraud proofs (or tendermint like evidence)

note to selves: specifying a formal mechanism for slashing is a perfect thing to do at the onsite.

evan-forbes

🚀

evan-forbes · 2022-08-31T12:13:13Z

docs/architecture/ADR-005-QGB_posting_confirms_in_reserved_namespace.md

+
+- Reduce significantly the gas fees paid by orchestrators.
+- Reduce significantly the use of Celestia state.
+- Keep the whole QGB confirms history via using block data, instead of being forced to delete it after the unbonding period to not end up with a super gigantic state. This would allow any QGB contract to have the whole history on any EVM chain.


Keep the whole QGB confirms history via using block data

not sure if this bullet point makes it clear that the old approach was also not deleting the transactions from previous blocks, so it technically could also do the same thing. The relayer design I think is what enables this

check now please?

docs/architecture/ADR-005-QGB_posting_confirms_in_reserved_namespace.md

evan-forbes · 2022-08-31T12:17:10Z

docs/architecture/ADR-005-QGB_posting_confirms_in_reserved_namespace.md

+## Context
+
+Currently, the QGB is using the state to save all the attestations and confirms it needs.
+We propose to post `Valset Confirms` and `DataCommitment Confirms` in a reserved namespace instead of adding them to the state to achieve the following:


Are we still planning on sticking the confirms in their own namespace? if not, I think we can update this to reflect that we're planning on simply posting them in the txs namespace instead of in their own.

check now please?

evan-forbes · 2022-08-31T12:19:17Z

docs/architecture/ADR-005-QGB_posting_confirms_in_reserved_namespace.md

+### Keep the existing design
+
+Keeping the current design would entail using the state extensively.
+This proves bad when the state grows after a few hundred attestations, and performing checks on the `Valset Confirms` and `DataCommitment Confirms` becomes too expensive.


not 100% sure state growth is the correct term here since the querying mechanism ended up being the culprit for the gas leak.

Also, we were planning on deleting the state after so long, so the state in theory would be constant

check now please?

docs/architecture/ADR-005-QGB_posting_confirms_in_reserved_namespace.md

rach-id · 2022-08-31T16:02:58Z

This also will need to be addressed in this ADR: #613 (comment)

Co-authored-by: CHAMI Rachid <chamirachid1@gmail.com>

rootulp

Overall LGTM. I defer to other reviewers for approval on this b/c my approval here doesn't carry much weight

docs/architecture/ADR-005-QGB_reduce_state_usage.md

rootulp · 2022-09-02T14:33:58Z

docs/architecture/ADR-005-QGB_reduce_state_usage.md

+
+For the orchestrators, they will also need to parse the history to keep track of any missed signatures. But, same as with the relayers, the cost is justified.
+
+For posting transactions, we will rely on gas fees as a mechanism to limit malicious parties to flood the network with invalid transactions. Then, eventually slash malicious behavious. However, since posting confirms will be possible for any user of the network. It won't be possible to slash ordinary users, who are not running validators, if they post invalid confirms.


[no change needed]

If we assume that the QGB data (i.e. Valset Confirms and DataCommitment Confirms) is stored in a non-reserved namespace, I think the problem stated above == https://forum.celestia.org/t/woods-attack-on-celestia/59

If the QGB data is only stored in transaction data and this document proposed adding new transaction types (distinct from MsgPayForData), I think we need a follow-up issue to discuss the metering (i.e. gas accounting) for these new message types.

If we assume that the QGB data (i.e. Valset Confirms and DataCommitment Confirms) is stored in a non-reserved namespace, I think the problem stated above == https://forum.celestia.org/t/woods-attack-on-celestia/59

We are no longer posting in confirms in namespaces, so woods attack is not relevant, if I understand right.

If the QGB data is only stored in transaction data and this document proposed adding new transaction types (distinct from MsgPayForData), I think we need a follow-up issue to discuss the metering (i.e. gas accounting) for these new message types.

No need for a new message type, we already have them defined and used. And the gas usage will be minimal since there will be no read/write in state.

Co-authored-by: Rootul Patel <rootulp@gmail.com>

evan-forbes

Nice!! excited about this change.

I don't think its necessary at all, since this ADR is focused on the state machine, but after we flesh out the implementation for the orchestrator and relayer, we might want to add a few more details into how that's done. Very optional in my opinion tho, as this serves the purpose of documenting the decision.

add the ADR 005 for posting confirms in reserved namespaces

66e8fb2

rach-id added documentation Improvements or additions to documentation enhancement New feature or request C: QGB labels Aug 24, 2022

rach-id requested review from rootulp and evan-forbes August 24, 2022 12:48

rach-id self-assigned this Aug 24, 2022

linter

9d30f66

rach-id requested review from adlerjohn and liamsi August 24, 2022 12:58

rootulp reviewed Aug 25, 2022

View reviewed changes

rach-id and others added 5 commits August 25, 2022 09:56

Update docs/architecture/ADR-005-QGB_posting_confirms_in_reserved_nam…

15d41ff

…espace.md Co-authored-by: Rootul Patel <rootulp@gmail.com>

Update docs/architecture/ADR-005-QGB_posting_confirms_in_reserved_nam…

f3f8508

…espace.md Co-authored-by: Rootul Patel <rootulp@gmail.com>

explains the fixed sized state

561637d

finish sentence

9d7ed7d

Apply suggestions from code review

3b978b8

Co-authored-by: Rootul Patel <rootulp@gmail.com>

rach-id requested a review from rootulp August 25, 2022 08:14

rootulp reviewed Aug 25, 2022

View reviewed changes

evan-forbes reviewed Aug 25, 2022

View reviewed changes

evan-forbes reviewed Aug 31, 2022

View reviewed changes

update the ADR to reflect the keeping the confirms in txs decision

b110920

rach-id requested review from rootulp and evan-forbes September 2, 2022 00:29

evan-forbes and others added 2 commits September 1, 2022 22:13

wording

bc6f050

Co-authored-by: CHAMI Rachid <chamirachid1@gmail.com>

linter

35ac8bd

rach-id changed the title ~~QGB ADR-005 Posting confirms in reserved namespaces~~ QGB ADR-005 Reducing QGB module state usage Sep 2, 2022

rename file

bb65497

rootulp reviewed Sep 2, 2022

View reviewed changes

Update docs/architecture/ADR-005-QGB_reduce_state_usage.md

2c2b175

Co-authored-by: Rootul Patel <rootulp@gmail.com>

unnecessary line

c0c45ab

evan-forbes approved these changes Sep 2, 2022

View reviewed changes

rach-id mentioned this pull request Sep 5, 2022

QGB decide on keeping the history of attestations #683

Closed

rach-id merged commit 7fda428 into celestiaorg:qgb-integration Sep 5, 2022

rach-id deleted the adr_5_confirms_in_reserved_namespace branch September 5, 2022 07:44

rach-id mentioned this pull request Sep 5, 2022

QGB implement the confirms in transactions design #684

Closed

This was referenced Oct 3, 2022

Question: State machine panic when querying invalid data #483

Closed

QGB investigate indexing attestation confirms in multiple ways #552

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QGB ADR-005 Reducing QGB module state usage #657

QGB ADR-005 Reducing QGB module state usage #657

rach-id commented Aug 24, 2022

rootulp left a comment

rootulp Aug 25, 2022

rach-id Aug 25, 2022

rootulp Aug 25, 2022

rach-id Aug 25, 2022

rootulp left a comment

rootulp Aug 25, 2022

rach-id Sep 2, 2022

evan-forbes left a comment •

edited

Loading

evan-forbes left a comment

evan-forbes Aug 31, 2022

rach-id Sep 2, 2022

evan-forbes Aug 31, 2022

rach-id Sep 2, 2022

evan-forbes Aug 31, 2022

rach-id Sep 2, 2022

rach-id commented Aug 31, 2022

rootulp left a comment

rootulp Sep 2, 2022

rach-id Sep 2, 2022

evan-forbes left a comment

	This proves bad when the state grows after a few hundred attestations, and performing checks on the `Valset Confirms` and `DataCommitment Confirms` becomes so expensive.
	This proves bad when the state grows after a few hundred attestations, and performing checks on the `Valset Confirms` and `DataCommitment Confirms` becomes too expensive.


		### Negative

		- Reducing the checks applied on the confirms: everyone will be able to post commitments, and we will have no way to sanitize them before adding them to a block. This would add an extra overhead for the relayer to choose the right commitments. However, it is alright, since we will not have a lot of Also, we will need to decide if this will be relayers, and we can give them enough computation power to compensate the overhead.


		## Context

		Currently, the QGB is using the state to save all the attestations and confirms it needs.


		For the orchestrators, they will also need to parse the history to keep track of any missed signatures. But, same as with the relayers, the cost is justified.

		For posting transactions, we will rely on gas fees as a mechanism to limit malicious parties to flood the network with invalid transactions. Then, eventually slash malicious behavious. However, since posting confirms will be possible for any user of the network. It won't be possible to slash ordinary users, who are not running validators, if they post invalid confirms.

QGB ADR-005 Reducing QGB module state usage #657

QGB ADR-005 Reducing QGB module state usage #657

Conversation

rach-id commented Aug 24, 2022

rootulp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rootulp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes left a comment • edited Loading

Choose a reason for hiding this comment

evan-forbes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rach-id commented Aug 31, 2022

rootulp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes left a comment

Choose a reason for hiding this comment

evan-forbes left a comment •

edited

Loading