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

Message index/count refactor - Part 1 #2535

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

Message index/count refactor - Part 1 #2535

wants to merge 51 commits into from

Conversation

diegoximenes
Copy link
Contributor

@diegoximenes diegoximenes commented Jul 30, 2024

Resolve NIT-2569

Different parts of Nitro (TransactionStreamer, InboxTracker, ExecutionEngine, Broadcaster, BlockValidator, etc), need to access the number of messages, or the content of a message, or the result related to processing a message.
Those properties are accessed in different ways through Nitro's code base, decreasing code readability.
Examples:

  • In order to get the result related to processing a message, BlockValidator calls TransactionStreamer.ResultAtCount passing the message index plus one. However, in order to get the result related to processing a message, TransactionStreamer calls ExecutionEngine.ResutlAtPos passing the message index as argument.
  • Variables that identify a message are named differently across the code base, such as pos, num, seq, index.

While reading the code base it is not uncommon to confuse what is the semantic meaning of a variable, such as if it is holding the number of messages, or a message identifier.

This PR is the first part of a refactor that unifies how properties related to messages are accessed through Nitro.
This PR mainly focus on refactoring TransactionStreamer, ExecutionEngine and Broadcaster.
It decreases message count usage in favor of using message indexes, such as declaring func (n *ExecutionNode) Reorg(newHeadMsgIdx arbutil.MessageIndex, ..., instead of func (n *ExecutionNode) Reorg(count arbutil.MessageIndex, ....
It names message indexes identifiers as msgIdx.

If the approach of this PR is approved, then later more PRs are going to be created with the following goals:

  • Spread the same approach to InboxTracker, BlockValidator, etc.
  • Create a arbutil.MessageCount type, as uint64. In this way it will be easier to identify the semantic meaning of a variable.
  • Represent delayed messages indexes as arbutil.MessageIndex.

@cla-bot cla-bot bot added the s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA. label Jul 30, 2024
@diegoximenes diegoximenes changed the title Message index/count refactor Message index/count refactor - Part 1 Jul 30, 2024
@diegoximenes diegoximenes marked this pull request as ready for review July 30, 2024 20:23
Comment on lines +6 to 11
// messages are 0-indexed
type MessageIndex uint64

func BlockNumberToMessageCount(blockNumber uint64, genesisBlockNumber uint64) MessageIndex {
return MessageIndex(blockNumber + 1 - genesisBlockNumber)
}
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about having a single type that represents the position which is what you pass around, and then having accessors for .Index() and .Count()?

You could even wrap it like this so you don't accidentally do some illegal operation with it.

type MessageIndex struct {
  inner uint64
}

I'm interested to hear others' thoughts too on this part of the PR since it's the core of the design.

Copy link
Contributor Author

@diegoximenes diegoximenes Aug 2, 2024

Choose a reason for hiding this comment

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

But what is the meaning of .Count()?
Is it the current length of the messages container, the length of the messages container that the caller expects, or just .Index() + 1?

Copy link
Contributor

@tsahee tsahee left a comment

Choose a reason for hiding this comment

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

initial thoughts

arbnode/transaction_streamer.go Outdated Show resolved Hide resolved
arbnode/inbox_tracker.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants