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

Runtime: Core BPF Migration: Setup migration configurations #525

Merged
merged 3 commits into from
Apr 4, 2024

Conversation

buffalojoec
Copy link

@buffalojoec buffalojoec commented Apr 1, 2024

This is chunk 6/7 of the broken up PR #79.

Problem

We're almost ready to configure the runtime's feature activation process to be
able to migrate builtin programs to Core BPF. However, we need to set up the
BuiltinPrototype object to store the migration configurations, and we need a
way to truly test these migrations on feature activation.

Summary of Changes

Add a field core_bpf_migration_config: Option<CoreBpfMigrationConfig> to the
BuiltinPrototype struct to house migration configurations.

Add a test-only module (mod test_only) to the builtins module that will
set the new core_bpf_migration_config field on the static BUILTINS and
STATELESS_BUILTINS lists to Some(..), containing values we can use to test
feature activations.

As with the previous PR in this series, this change remains off the hot path.

@buffalojoec buffalojoec changed the title Runtime: Core BPF Migration: Setup for testing feature activations Runtime: Core BPF Migration: Setup for feature activations Apr 1, 2024
@buffalojoec buffalojoec changed the title Runtime: Core BPF Migration: Setup for feature activations Runtime: Core BPF Migration: Setup migration configurations Apr 1, 2024
@codecov-commenter
Copy link

codecov-commenter commented Apr 1, 2024

Codecov Report

Attention: Patch coverage is 94.54545% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 81.8%. Comparing base (2c11b7a) to head (5019f95).
Report is 5 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master     #525    +/-   ##
========================================
  Coverage    81.8%    81.8%            
========================================
  Files         849      850     +1     
  Lines      229183   229305   +122     
========================================
+ Hits       187585   187704   +119     
- Misses      41598    41601     +3     

Copy link

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Hmm, I don't love how this turned out, with duplicate fields on every prototype. While I don't totally have my heart set on this, could we hide the messiness behind a macro?
Something like:

testable_prototype!(
    BuiltinPrototype {
        core_bpf_migration_config: None,
        enable_feature_id: None,
        ...
    }
),
...

which expands to this existing code, using the name field to id the CONFIG module. Wdyt?

runtime/src/bank/builtins/mod.rs Show resolved Hide resolved
@buffalojoec
Copy link
Author

buffalojoec commented Apr 4, 2024

@CriesofCarrots Yeah I don't love the duplicate fields, either, tbh. It felt like a significant step up from having two of each list, though.

The macro idea is neat! Are you thinking something like this?

macro_rules! testable_prototype {
    ($prototype:ident {
        core_bpf_migration_config: $core_bpf_migration_config:expr,
        name: $name:expr,
        $($field:ident : $value:expr),* $(,)?
    }) => {
        $prototype {
            core_bpf_migration_config: {
                #[cfg(not(test))]
                {
                    $core_bpf_migration_config
                }
                #[cfg(test)]
                {
                    Some( ::test_only::$name::CONFIG )
                }
            },
            name: $name,
            $($field: $value),*
        }
    };
}

Messing around with this, the one tricky part I foresee is wrestling a string literal into a path identifier, unless I'm missing something obvious?

@CriesofCarrots
Copy link

CriesofCarrots commented Apr 4, 2024

Are you thinking something like this?

Yes! Something like that.

Messing around with this, the one tricky part I foresee is wrestling a string literal into a path identifier, unless I'm missing something obvious?

The easiest approach might be to pass the name as an ident, and use stringify to populate the name field. This cabbagey playground demonstrates (the CONFIG global path definition is probably not right; just the quickest thing I could get to work).

@buffalojoec
Copy link
Author

The easiest approach might be to pass the name as an ident, and use stringify to populate the name field. This cabbagey playground demonstrates (the CONFIG global path definition is probably not right; just the quickest thing I could get to work).

Alright nice, thanks for throwing that sandbox together. I went ahead and implemented it as suggested, and also added a little test for it!

@buffalojoec buffalojoec force-pushed the core-bpf-migrate-setup branch from 08b75cb to 5019f95 Compare April 4, 2024 19:52
Copy link

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Rad, thanks for the rework! This feels a lot less brittle to me.

@buffalojoec buffalojoec merged commit 87fc227 into anza-xyz:master Apr 4, 2024
38 checks passed
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.

3 participants