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

L2+ testing todos #1223

Closed
cryptoAtwill opened this issue Dec 2, 2024 · 5 comments
Closed

L2+ testing todos #1223

cryptoAtwill opened this issue Dec 2, 2024 · 5 comments
Assignees

Comments

@cryptoAtwill
Copy link
Contributor

cryptoAtwill commented Dec 2, 2024

After an initial round of testing with L2+ feature, here is a list of todos to consider:

  1. Adding a doc on who, how is invoking cross messages calls. Because there were some assumption/requirements not properly documented yet, all the components needed to ensure correct functioning.
  2. Currently, there is missing a debug tool for cross message calls. It would be really helpful to have a tool, maybe external, that tracks the lifecycle of the cross network messages, i.e. where the message submitted, which subnet is the message now, is the message committed into the postbox and etc... This would help debug cross network message execution.
  3. Checking subnet.circSupply. Not sure if this variable is tracked correctly. This value is deducted by IPCEnvelop.value regardless message type? But only increased (after bootstrap) when message is Transfer
  4. There should be a debug tool to check if the message execution is successful or failed. If failed, should be possible to show the error.
  5. Consistent way to encode ret bytes. The way that encodes the error is using encodeWithSelector, but for L2+ error is just encode, it's kind of confusing for debugging to tell which decode method to use.
  6. Emit event in test should be removed. Events should be emitted in actual code, not from test, otherwise the expect becomes useless.
@karlem
Copy link
Contributor

karlem commented Dec 2, 2024

  1. 100% agree. I will work on it today.
  2. I think we could use https://thegraph.com/cs/ for this
  3. This looks like a bug. Will fix today.

@cryptoAtwill
Copy link
Contributor Author

Because of #1225, the current approach of scanning events emitted from postbox will not work. There will be missing events.

@cryptoAtwill
Copy link
Contributor Author

cryptoAtwill commented Dec 3, 2024

The nonce handling is incorrect. Only the first xnet message will work, subsequent ones will fail.

The scenario is sending TWO cross network message from L1 to L3. This is the first ever L1 topdown message, so for the cross message itself, the nonce is 0.

On L2, topdown finality will have the message applied. It gets committed with appliedNonce increase by 1. Then the committed message in postbox is propagated. Internally it calls commitValidatedCrossMessage and appliedNonce is incremented again by 1.

The overall effect is one xnet message from L1 to L3 will increase the applied nonce by 2. This is observed when testing in calibration.

The second cross network message from L1 to L3 will have nonce == 1, the nonce will not match.

@cryptoAtwill
Copy link
Contributor Author

Potential double nonce increment.

Similar to the above, the appliedNonce is incremented first here. But subsequently, it also call sendReceipt. Internally, it will increment the appliedNonce by 1 as well. This will lead to one message increasing the nonce by 2.

@raulk raulk assigned cryptoAtwill and karlem and unassigned cryptoAtwill Dec 4, 2024
@karlem
Copy link
Contributor

karlem commented Dec 5, 2024

Closing this issue in favor of PR #1157, as the TODOs have been migrated there.

@karlem karlem closed this as completed Dec 5, 2024
@github-project-automation github-project-automation bot moved this from Backlog to Done in IPC Core Development Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

2 participants