Skip to content

deps: Remove SDK Fork #79

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

Merged
merged 16 commits into from
May 9, 2025
Merged

deps: Remove SDK Fork #79

merged 16 commits into from
May 9, 2025

Conversation

Eric-Warehime
Copy link
Contributor

Description

Removes the fork of the cosmos sdk--only dependency here is the Copy function.

Closes: #XXXX


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • tackled an existing issue or discussed with a team member
  • left instructions on how to review the changes
  • targeted the main branch

Reviewers Checklist

All items are required.
Please add a note if the item is not applicable
and please add your handle next to the items reviewed
if you only reviewed selected items.

I have...

  • added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • confirmed all author checklist items have been addressed
  • confirmed that this PR does not change production code
  • reviewed content
  • tested instructions (if applicable)
  • confirmed all CI checks have passed

@Pitasi
Copy link

Pitasi commented Apr 16, 2025

x/evm limits the number of precompiles call made from a contract to the arbitrary number 7:

evm/x/vm/types/call.go

Lines 12 to 15 in a48a796

// MaxPrecompileCalls is the maximum number of precompile
// calls within a transaction. We want to limit this because
// for each precompile tx we're creating a cached context
const MaxPrecompileCalls uint8 = 7

I think the comment is related to this (I might be wrong though).

Does this change make it any better, or does it not impact this at all? I don't know if changing from Copy() to CacheMultiStore() affects performances.

@Eric-Warehime
Copy link
Contributor Author

x/evm limits the number of precompiles call made from a contract to the arbitrary number 7:

evm/x/vm/types/call.go

Lines 12 to 15 in a48a796

// MaxPrecompileCalls is the maximum number of precompile
// calls within a transaction. We want to limit this because
// for each precompile tx we're creating a cached context
const MaxPrecompileCalls uint8 = 7

I think the comment is related to this (I might be wrong though).

Does this change make it any better, or does it not impact this at all? I don't know if changing from Copy() to CacheMultiStore() affects performances.

It seems like the number of precompile calls within a given txn is tracked at the StateDb object level here but the change away from using Copy should improve performance because we're using the SDK's branching contexts now instead of doing a deep copy of the store.

@Eric-Warehime Eric-Warehime marked this pull request as ready for review April 16, 2025 18:26
Copy link
Member

@vladjdk vladjdk left a comment

Choose a reason for hiding this comment

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

Looks fine to me - I read the test that breaks if you don't use Copy() or CacheMultiStore(), and it seems like either method would fix it. I don't think we need to be making a deep copy of the store, using CacheMultiStore on a CacheMultiStore-generated object looks like it works. Should be more efficient too :)

// Would probably want more tests on this eventually though.

@zsystm
Copy link
Contributor

zsystm commented Apr 18, 2025

@Eric-Warehime @vladjdk

One key difference I've confirmed between CacheMultiStore() and Copy() is especially critical when reasoning about cache layering and ephemeral state behavior.

CacheMultiStore()

  • Creates a new cache layer on top of the current store.
  • The parent and child share the same ephemeral (in-memory) state.
  • Updates made in the parent store—even after the child cache is created—are visible to the child (in our environment, even after the parent Commit()).
  • Conversely, updates made in the child will be merged back into the parent when childCMS.Write() is called.
  • When chaining multiple layers (e.g. grandchild), changes flow upward only when .Write() is called at each layer.

Copy()

  • Produces a fully isolated snapshot of the current ephemeral state.
  • The copy does not see any future updates from the original store.
  • Changes made in the copy are not reflected in the original.
  • Calling copyCMS.Write() merges changes directly to the parent KVStore, bypassing any intermediate cache layers (e.g., the original CMS).

This distinction is subtle but has significant implications.
Replacing Copy() with CacheMultiStore() changes the behavior in critical ways:

  • Original cache changes will now affect the new layer.
    Previously, the copied store was isolated. With CacheMultiStore(), any new writes in the parent cache will be reflected in the child, which could unintentionally override expected state.
  • Child writes now propagate upward
    In the Copy() version, writes only touched the parent KVStore. With CacheMultiStore(), calling .Write() will modify the original cache’s ephemeral state—potentially overwriting intermediate values.
  • Unexpected interactions between layers.
    Cache hierarchy behavior becomes more complex, and unexpected state mutations may occur if the layering and write flow aren't clearly understood.

Unless shared cache behavior is explicitly intended and carefully controlled, this change can introduce non-obvious side effects. I’d recommend reviewing whether this change is safe across all usage paths—especially in areas where .Write() is involved.

Reference test cases:

Copy link
Member

@vladjdk vladjdk left a comment

Choose a reason for hiding this comment

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

I'm gonna change my response based on our conversation we had earlier. Let's add a test to make sure that assures that parent state never changes (or affects the child state) during the time we're using the CMS, and also make sure that we never make use of any intermediate CMS layers since writing to the grandchild pushes straight to the root.

@Eric-Warehime
Copy link
Contributor Author

I've added some tests using existing precompiles which tracks cachemultistore usage and tracks number and sequencing of writes. I'm personally satisfied with the behavior. Let me know if there are extra scenarios we want tested.

@Eric-Warehime Eric-Warehime requested a review from vladjdk May 8, 2025 01:51
Copy link
Contributor

@zsystm zsystm left a comment

Choose a reason for hiding this comment

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

@Eric-Warehime
I tested several scenarios using the following Solidity contracts to validate that snapshot and revert behaves correctly under various execution paths — especially after the migration from Copy-based MultiStore to CacheMultiStore.

Solidity test contracts used for validation
pragma solidity >=0.8.17;

interface DistributionI {
    function claimRewards(address delegatorAddress, uint32 maxRetrieve) external returns (bool success);
    function setWithdrawAddress(address delegatorAddress, string memory withdrawerAddress) external returns (bool success);
    function fundCommunityPool(address from, uint256 amount) external returns (bool success);
}

address constant DISTRIBUTION_PRECOMPILE_ADDRESS = 0x0000000000000000000000000000000000000801;
DistributionI constant DISTRIBUTION_CONTRACT = DistributionI(DISTRIBUTION_PRECOMPILE_ADDRESS);

contract DistributionCaller {
    /**
     * Calls claimRewards from the precompile. Even if it fails,
     * the call is caught and false is returned — no top-level revert.
     */
    function callClaimRewards(address delegatorAddress, uint32 maxRetrieve) external returns (bool) {
        bool success;
        try DISTRIBUTION_CONTRACT.claimRewards(delegatorAddress, maxRetrieve) returns (bool result) {
            success = result;
        } catch {
            success = false;
        }
        return success;
    }
}

contract DistributionMultiCallRevertTest {
    /**
     * Calls setWithdrawAddress and claimRewards, then forcefully reverts.
     * All state changes must be rolled back.
     */
    function multiCallThenRevert(
        address delegator,
        string memory newWithdrawAddress,
        uint32 maxRetrieve
    ) external {
        bool ok1 = DISTRIBUTION_CONTRACT.setWithdrawAddress(delegator, newWithdrawAddress);
        require(ok1, "setWithdrawAddress failed");

        bool ok2 = DISTRIBUTION_CONTRACT.claimRewards(delegator, maxRetrieve);
        require(ok2, "claimRewards failed");

        revert("Force revert for testing ephemeral rollback");
    }
}

contract DistributionMultiCallSuccessTest {
    /**
     * Executes three successful operations:
     * 1. claimRewards
     * 2. fundCommunityPool
     * 3. setWithdrawAddress
     * All changes should be committed.
     */
    function multiCallSuccess(
        address delegator,
        uint32 maxRetrieve,
        address from,
        uint256 amount,
        string memory newWithdrawAddress
    ) external {
        bool ok1 = DISTRIBUTION_CONTRACT.claimRewards(delegator, maxRetrieve);
        require(ok1, "claimRewards failed");

        bool ok2 = DISTRIBUTION_CONTRACT.fundCommunityPool(from, amount);
        require(ok2, "fundCommunityPool failed");

        bool ok3 = DISTRIBUTION_CONTRACT.setWithdrawAddress(delegator, newWithdrawAddress);
        require(ok3, "setWithdrawAddress failed");
    }
}

contract ReentrantSetWithdrawAddressTest {
    /**
     * Tests reentrant behavior. If doReenter is true,
     * the function calls itself once more. Used to confirm
     * that state changes are properly isolated and reverted
     * if any reentrant call fails.
     */
    function reentrantSetWithdrawAddress(
        address delegator,
        string memory newWithdrawAddress,
        bool doReenter
    ) external {
        bool ok = DISTRIBUTION_CONTRACT.setWithdrawAddress(delegator, newWithdrawAddress);
        require(ok, "setWithdrawAddress failed");

        if (doReenter) {
            this.reentrantSetWithdrawAddress(delegator, newWithdrawAddress, false);
        }
    }
}

The focus was to ensure:

  • Snapshots are not accidentally overwritten during nested calls or intermediate state changes.
  • State is fully reverted on a top-level revert, even if earlier precompile calls have already mutated state.

All tests completed without issues.
From what I can tell, CacheMultiStore behaves correctly in these scenarios, and I don’t see any major concerns at this point — just a few minor comments left.

)

// TrackingMultiStore implements the CacheMultiStore interface, but tracks calls to the Write interface as
// well ass the branches created via CacheMultiStore()
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

Suggested change
// well ass the branches created via CacheMultiStore()
// well as the branches created via CacheMultiStore()

Comment on lines 15 to 20
type TrackingMultiStore struct {
Store storetypes.CacheMultiStore
Writes int
WriteTS *time.Time
HistoricalStores []*TrackingMultiStore
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add some comments like the following for helping other devs understand the test code well?

Suggested change
type TrackingMultiStore struct {
Store storetypes.CacheMultiStore
Writes int
WriteTS *time.Time
HistoricalStores []*TrackingMultiStore
}
type TrackingMultiStore struct {
// Store is the underlying CacheMultiStore being wrapped and tracked.
Store storetypes.CacheMultiStore
// Writes is the number of times Write() has been called on this store.
Writes int
// WriteTS is the timestamp of the last Write() call, used to determine write order.
WriteTS *time.Time
// HistoricalStores holds any CacheMultiStores created from this store via CacheMultiStore().
// Each represents a new *branch* of the same logical root, not a hierarchical child.
// Branches are tracked in order of creation, but have no implied depth or parent-child relationship.
HistoricalStores []*TrackingMultiStore
}

@@ -276,3 +277,196 @@ func (s *PrecompileTestSuite) TestRun() {
})
}
}

func (s *PrecompileTestSuite) TestCMS() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that staking_test.go includes failure cases (like gas too low or invalid method inputs) to validate negative paths.
Would it make sense to include a few similar test cases here as well?

{
"fail - contract gas limit is < gas cost to run a query / tx",
func(delegator, grantee testkeyring.Key) []byte {
// TODO: why is this required?
Copy link
Member

Choose a reason for hiding this comment

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

This won't be required after #62 is merged. Let's get that in before this one

Copy link
Member

Choose a reason for hiding this comment

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

Merged, these authorizations should no longer be needed

@Eric-Warehime Eric-Warehime merged commit 59a0491 into main May 9, 2025
17 checks passed
@Eric-Warehime Eric-Warehime deleted the eric/remove-sdk-fork branch May 9, 2025 21:18
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.

4 participants