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 new object_per_epoch_marker_table that includes consensus start versions #20822

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

aschran
Copy link
Contributor

@aschran aschran commented Jan 8, 2025

This introduces the concept of a "FullObjectID" and "FullObjectKey", which for consensus objects includes the initial shared version/start version.

Migrates to these new types across the codebase where relevant.


Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.

For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.

  • Protocol:
  • Nodes (Validators and Full nodes):
  • gRPC:
  • JSON-RPC:
  • GraphQL:
  • CLI:
  • Rust SDK:

Copy link

vercel bot commented Jan 8, 2025

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

Name Status Preview Comments Updated (UTC)
sui-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 14, 2025 7:28pm
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Jan 14, 2025 7:28pm
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Jan 14, 2025 7:28pm
sui-typescript-docs ⬜️ Ignored (Inspect) Visit Preview Jan 14, 2025 7:28pm

… versions

This introduces the concept of a "FullObjectID" and
"FullObjectKey", which for consensus objects includes the
initial shared version/start version.

Migrates to these new types across the codebase where relevant.
@@ -303,6 +307,8 @@ pub trait ObjectCacheRead: Send + Sync {
input_key
);
// If the key exists at the specified version, then the object is available.
// TODO-DNS does this need to be inverted to check markers first, so that it doesn't incorrectly load
Copy link
Contributor

Choose a reason for hiding this comment

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

DNS comment

Copy link
Contributor

Choose a reason for hiding this comment

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

and i think the answer is yes, clearly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

noting for posterity per offline discussion - this function is correct as-is, and when implementing execution logic for consensusv2 we'll need to add code to check after the object is loaded that the object owner is what we expect based on tx input args

@@ -43,6 +43,7 @@ pub struct TemporaryStore<'backing> {
store: &'backing dyn BackingStore,
tx_digest: TransactionDigest,
input_objects: BTreeMap<ObjectID, Object>,
deleted_consensus_objects: BTreeMap<ObjectID, SequenceNumber>,
Copy link
Contributor

Choose a reason for hiding this comment

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

why do older execution versions need this new code? they don't appear to use it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they use it to construct the InnerTemporaryStore which is not part of the versioned sui-execution crate

version,
self.current_epoch_id,
self.protocol_config
.use_object_per_epoch_marker_table_v2_as_option()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: isn't there a getter without the option that would just return false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so? from the macro:

                        let getter = quote! {
                            // Derive the getter
                            pub fn #field_name(&self) -> #inner_type {
                                self.#field_name.expect(Self::CONSTANT_ERR_MSG)
                            }

                            pub fn #as_option_name(&self) -> #field_type {
                                self.#field_name
                            }
                        };

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

Successfully merging this pull request may close these issues.

2 participants