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

Update AnyTrust docs with links and refinements #1715

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

epociask
Copy link

No description provided.

@cla-bot cla-bot bot added the s label Sep 30, 2024
Copy link

vercel bot commented Sep 30, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
arbitrum-docs ✅ Ready (Inspect) Visit Preview Oct 3, 2024 8:51pm

Copy link
Contributor

@TucksonDev TucksonDev left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! 🙏

I've added a few comments here and there. I'd recommend avoiding changes to concept names that are used throughout the document, since those might also be referenced in other pages of the docs, so changing only this instance might create unnecessary complexity in the language.


- the number of Committee members, and
- for each Committee member, a BLS public key, and
- the number of Committee signatures required.

Keysets are identified by their hashes.

An L1 KeysetManager contract maintains a list of currently valid Keysets. The L2 chain's Owner can add or remove Keysets from this list. When a Keyset becomes valid, the KeysetManager contract emits an L1 Ethereum event containing the Keyset's hash and full contents. This allows the contents to be recovered later by anyone, given only the Keyset hash.
The SequencerInbox contract maintains `dasKeySetInfo` mapping with embedded [access controls](https://github.com/OffchainLabs/nitro-contracts/blob/fbbcef09c95f69decabaced3da683f987902f3e2/src/bridge/SequencerInbox.sol#L750-L778) for adding new keysets and invalidating existing ones. Upon addition, a L1 system contract [event](https://github.com/OffchainLabs/nitro-contracts/blob/fbbcef09c95f69decabaced3da683f987902f3e2/src/bridge/ISequencerInbox.sol#L197-L201) containing the Keyset's hash and full contents is emitted and processed by the L2 chain during [validation](https://github.com/Layr-Labs/nitro/blob/eigenda-v3.2.1/arbstate/daprovider/util.go#L194).
Copy link
Contributor

Choose a reason for hiding this comment

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

A few things about this paragraph:

  • I believe it's clearer to mention that the chain owner is the one able to add or remove keysets from the mapping (instead of mentioning access controls)
  • Since you're referencing the SequencerInbox at the beginning, you can do so too when mentioning the event that's emitted (right now it says a L1 system contract).
  • Those events are not only exactly processed by validators. They are emitted and later used to verify new signed certificates. In any case, the original text was clearer in that regard.
  • The new text includes links to repositories that are not maintained by Arbitrum or OCL (Layr-Labs/nitro)


Although the API does not limit the number of Keysets that can be valid at the same time, normally only one Keyset will be valid.
Although the API does not limit the number of active Keysets that can be valid at the same time, normally only one Keyset will be valid.
Copy link
Contributor

Choose a reason for hiding this comment

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

Active keysets that can be valid seems a bit redundant. I'd remove active from here.


## Data Availability Certificates

A central concept in AnyTrust is the Data Availability Certificate (hereafter, a "DACert"). A DACert contains:
A central concept in AnyTrust is the Data Availability Certificate (hereafter, a "DACert"). A [DACert](https://github.com/Layr-Labs/nitro/blob/eigenda-v3.2.1/arbstate/daprovider/util.go#L239-L246) contains:
Copy link
Contributor

Choose a reason for hiding this comment

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

Reference to a repo that's not maintained by Arbitrum or OCL


Because of the 2-of-N trust assumption, a DACert constitutes proof that the block's data (i.e., the preimage of the hash in the DACert) will be available from at least one honest Committee member, at least until the expiration time.
Because of the `1-of-N` trust assumption, a DACert constitutes proof that the batch data (i.e., the preimage of the hash in the DACert) will be available from at least one honest Committee member, at least until the expiration time.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Because of the `1-of-N` trust assumption, a DACert constitutes proof that the batch data (i.e., the preimage of the hash in the DACert) will be available from at least one honest Committee member, at least until the expiration time.
Because of the `2-of-N` trust assumption, a DACert constitutes proof that the block's data (i.e., the preimage of the hash in the DACert) will be available from at least one honest Committee member, at least until the expiration time.

We are using the same language throughout the rest of the page, so I wouldn't change this instance alone. If we want to change "data block" to "batch" we would have to do so in all instances.


In ordinary (non-AnyTrust) Nitro, the Arbitrum sequencer posts data blocks on the L1 chain as calldata. The hashes of the data blocks are committed by the L1 Inbox contract, allowing the data to be reliably read by L2 code.
The hashes of the data blocks are committed by the L1 chain's Sequencer Inbox contract, allowing the data to be reliably read by L2 chain nodes.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't remove the first part of the paragraph. It makes sense to reference the way non-AnyTrust nitro works, to be compared with AnyTrust.


AnyTrust gives the sequencer two ways to post a data block on L1: it can post the full data as above, or it can post a DACert proving availability of the data. The L1 inbox contract will reject any DACert that uses an invalid Keyset; the other aspects of DACert validity are checked by L2 code.
AnyTrust gives the sequencer an additional way to post batches to the L1 chain. The L1 Sequencer Inbox contract will reject any DACert that uses an invalid Keyset; other certificate validity checks are enforced during derivation.
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to specify what's the additional way to post a batch.


The L2 code that reads data from the inbox reads a full-data block as in ordinary Nitro. If it sees a DACert instead, it checks the validity of the DACert, with reference to the Keyset specified by the DACert (which is known to be valid because the L1 Inbox verified that). The L2 code verifies that
The L2 code that reads data from the inbox reads a full-data block as in ordinary Nitro. If it sees a DACert instead, it checks the validity of the DACert, with reference to the Keyset specified by the DACert (which is known to be valid since the Sequencer Inbox verified that). The L2 chain code verifies that:
Copy link
Contributor

Choose a reason for hiding this comment

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

SequencerInbox is usually written without spaces.

We should be consistent with the language. L2 code is a valid expression, but if we want to change it to another thing, we would have to change all instances.

In any case, my recommendation for an external contribution is not to change language that is correct, even if there might be a better way to express that concept, since we take a global approach within the docs, and try to use the same language across all pages. The best way to deal with changing language is to open an issue so we can discuss internally how to approach the change 🙏


- the number of signers is at least the number required by the Keyset, and
- the aggregated signature is valid for the claimed signers, and
- the expiration time is at least two weeks after the current L2 timestamp.

If the DACert is invalid, the L2 code discards the DACert and moves on to the next data block. If the DACert is valid, the L2 code reads the data block, which is guaranteed to be available because the DACert is valid.
If the DACert is invalid, the L2 code discards the DACert and moves on to the next data block. If the DACert is valid, the L2 code reads the data block, which is guaranteed to be available because the DACert is valid. There are no significant changes to fraud proving since these validations are compiled into the wasm replay script - removing the need for additional opcodes or changes to one step proving for a `READSEQUENCERINBOX` opcode.
Copy link
Contributor

Choose a reason for hiding this comment

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

The last sentence of the paragraph seems a bit irrelevant in this context. The whole AnyTrust page refers only to the way the information is stored, which makes sense since it's the only thing that changes.

If we want to add a note about the potential impact on fraud proving, it might be better to create a new section for it, but I don't think it's necessary to include it.

@@ -68,4 +68,4 @@ When the Arbitrum sequencer produces a data batch that it wants to post using th

Once the Sequencer has collected enough signatures, it can aggregate the signatures and create a valid DACert for the (hash, expiration time) pair. The Sequencer then posts that DACert to the L1 inbox contract, making it available to the AnyTrust chain software at L2.

If the Sequencer fails to collect enough signatures within a few minutes, it will abandon the attempt to use the Committee, and will "fall back to rollup" by posting the full data directly to the L1 chain, as it would do in a non-AnyTrust chain. The L2 software can understand both data posting formats (via DACert or via full data) and will handle each one correctly.
If the Sequencer fails to collect enough signatures within a few minutes, it will abandon the attempt to use the Committee, and will "fall back to rollup" by posting the full data directly to the L1 chain, as it would do in a non-AnyTrust chain. The Nitro software can understand both data posting formats (via DACert or via native Ethereum DA) and will handle each one correctly.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the first mention to "native Ethereum DA" and might be confusing for the reader. A better alternative would be via complete data).

@CLAassistant
Copy link

CLAassistant commented Nov 26, 2024

CLA assistant check
All committers have signed the CLA.

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

Successfully merging this pull request may close these issues.

4 participants