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

Add timeboosted information to broadcast feed #2695

Open
wants to merge 5 commits into
base: express-lane-timeboost
Choose a base branch
from

Conversation

ganeshvanahalli
Copy link
Contributor

@ganeshvanahalli ganeshvanahalli commented Sep 24, 2024

This PR adds a new field blockMetadata to the BroadcastFeedMessage struct that represents whether a particular transaction in the block was time-boosted or not.

The field blockMetadata is an array of bytes and it starts with a 0 (version) followed by ceil(N/8) number of bytes where N is the number of txs in the block. Basically nth bit of the kth byte of blockMetadata byte array represents the status of 8*(k-1)+(n+1) th transaction, if its 1 means the tx was timeboosted else it wasn't.
Example: if blockMetadata array viewed as bits is 00000000 01100101 then 2nd, 3rd, 6th and 8th transactions in the block were timeboosted.

Resolves NIT-2779

@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 Sep 24, 2024
@ganeshvanahalli ganeshvanahalli marked this pull request as ready for review September 24, 2024 12:49
Copy link
Member

@Tristan-Wilson Tristan-Wilson left a comment

Choose a reason for hiding this comment

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

Overall it looks good, just a few comments.

Comment on lines 60 to 64
maxTxCount := (len(m.Timeboosted) - 1) * 8
if txIndex >= maxTxCount {
return false
}
return m.Timeboosted[1+(txIndex/8)]&(1<<(txIndex%8)) != 0
Copy link
Member

Choose a reason for hiding this comment

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

I think it's worth making a special type for the []byte representing Timeboosted to encapsulate this functionality.

Copy link
Member

Choose a reason for hiding this comment

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

I think the idea behind the versioning is that what we're storing in this byte buffer is supposed to be block metadata which may grow to include more things than just whether txs were timeboosted or not. See NIT-2779. So it should be called block metadata or something similar, and schema.go should change too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, fixed it

Copy link
Member

Choose a reason for hiding this comment

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

What I mean is having the logic for checking if a tx is timeboosted being a function on the type you currently have called Timeboosted (which should probably also be called BlockMetadata. This goes for anywhere the is logic that cares about the bitwise representation of the block metadata.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh that makes a lot of sense, i'll rename the type and have the func defined on that

broadcaster/message/message_serialization_test.go Outdated Show resolved Hide resolved
execution/gethexec/executionengine.go Outdated Show resolved Hide resolved
execution/gethexec/executionengine.go Outdated Show resolved Hide resolved
system_tests/timeboost_test.go Outdated Show resolved Hide resolved
system_tests/timeboost_test.go Show resolved Hide resolved
arbnode/schema.go Outdated Show resolved Hide resolved
@ganeshvanahalli ganeshvanahalli changed the title Add timeboosted field to broadcast feed Add timeboosted information to broadcast feed Oct 2, 2024
arbnode/schema.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.

2 participants