x/core with core features #585

hacheigriega · 2025-08-13T14:36:01Z

Explanation of Changes

Implementation of x/core with full data request flow.

This PR only implements the messages types that are necessary to simulate a full data request flow. The rest of the messages will be implemented in a separate PR.
x/tally has been merged into x/core.
Adds a wrapper of x/wasm to continue supporting Core Contract messages that are required for data request fulfillment: AddToAllowList (may not be necessary), Stake, PostDataRequest, CommitDataResult, and RevealDataResult.

States

allowlist:    0x00                            -> []PublicKey
stakers:      0x01 | PublicKey                -> Staker
dataRequests: 0x02 | DR_ID                    -> DataRequest
revealBodies: 0x03 | DR_ID | PublicKey        -> RevealBody
committing:   0x04 | DataRequestIndex         -> ()
revealing:    0x05 | DataRequestIndex         -> ()
tallying:     0x06 | DataRequestIndex         -> ()
timeoutQueue: 0x07 | DR_ID | Timeout_Height   -> ()
params:       0x08                            -> Params

Messages

AddToAllowlist

Staking

Stake
SetStakingConfig -> UpdateParams

Data Requests

PostDataRequest
Commit
Reveal
SetDrConfig -> UpdateParams

Testing

(Write your test plan here)

Dependency Chain

Think about the changes (msgs, events, limits, etc.) made in this PR and see if you need to make an issue or a PR on the following repos.

Related PRs and Issues

Closes #587

hacheigriega · 2025-08-15T13:11:22Z

Oops, thought I fixed all interchain test fails, but I see there is one still failing. Looking into it now..

x/core/types/abci_types.go

gluax

Taking a break for now so I give this PR the attention to detail it deserves

go.mod

interchaintest/go.mod

proto/sedachain/core/v1/core.proto

gluax · 2025-08-15T16:18:48Z

x/core/keeper/endblock.go

+		if err != nil {
+			telemetry.SetGauge(1, types.TelemetryKeyDRFlowHalt)
+			k.Logger(ctx).Error("[HALTS_DR_FLOW] failed to retrieve data request", "err", err)
+			return nil
+		}


can this ever happen now that it's also stored in the module? Or ig we still have the shim right so it remains for a while?

gluax · 2025-08-15T16:23:41Z

x/core/keeper/endblock.go

+			return err
+		}
+
+		err = k.UpdateDataRequestIndexing(ctx, dr.Index(), dr.Status, types.DATA_REQUEST_STATUS_UNSPECIFIED)
+		if err != nil {
+			return err
+		}
+		dr.Status = types.DATA_REQUEST_STATUS_UNSPECIFIED
+
+		err = k.RemoveRevealBodies(ctx, dr.Id)
+		if err != nil {
+			return err
+		}
+		err = k.RemoveDataRequest(ctx, dr.Id)
+		if err != nil {
+			return err
+		}
+
+		dataResults[i].GasUsed = gasMeter.TotalGasUsed()
+		dataResults[i].Id, err = dataResults[i].TryHash()
+		if err != nil {
+			return err


these errors would full exit the processing... is that what we want? Not sure how chain side handled it before. The contract side only errored when it couldn't load the staking config, escrow, staaker or failed to set the response

For crucial errors like state write errors or hashing errors, we returned an error to halt the chain. If the contract returned an error, we caught the error without halting the chain.
Once we add pausability, we could also pause data request when a serious, unexpected error like this is encountered?

That's a good food for thought question. I'm not sure if we'd want to automatically do that, but there's merit in it too. Hmm

x/core/keeper/endblock.go

gluax · 2025-08-15T16:29:12Z

x/core/keeper/endblock.go

+
+	// Phase 1: Filtering
+	//nolint:gosec // G115: No overflow guaranteed by validation logic.
+	filterResult, filterErr := ExecuteFilter(reveals, dr.ConsensusFilter, uint16(dr.ReplicationFactor), params, gasMeter)


Oh I would prefer we don't cast fields every time we use them, and I assume we do the same for other similar fields. Is it painful to have the Protobuf struct have a validation method that would then ensure correct types?

gluax · 2025-08-15T16:29:20Z

x/core/keeper/endblock.go

+	StdOut       []string
+	StdErr       []string
+	Result       []byte
+	ExitCode     uint32


Ideally a uint8

The VM returns a uint32 value, so I think we should keep it until we change it from the VM side?

gluax

Next batch of review comments.

gluax · 2025-08-17T18:13:20Z

x/core/keeper/gas_meter.go

 	return dists
 }

+func (k Keeper) ProcessDistributions(ctx sdk.Context, dists []types.Distribution, dr *types.DataRequest, minimumStake math.Int) error {


Considering in endblock.go in the same module we call GetGasMeterResults and then immediately call this function- it would seem we can just merge them? I.e. no need to build the distribution list in the first place. Or do we ever plan to have logic in-between those two steps?

Yeah we could. It was just making testing difficult because it is difficult to check the distribution amounts.

I think it would be worth merging them, maybe we can make some test utilities to make testing that flow easier.

x/core/keeper/gas_meter.go

gluax · 2025-08-17T18:16:51Z

x/core/keeper/gas_meter.go

+			amount = math.MinInt(dist.DataProxyReward.Amount, remainingEscrow)
+			payoutAddr, err := sdk.AccAddressFromBech32(dist.DataProxyReward.PayoutAddress)
+			if err != nil {
+				// Should not be reachable because the address has been validated.


I know we aren't doing this in this PR. But maybe a log that says unreachable and reason. This way if it ever pops up in the logs we can appropiately panic lol

Well the validate function of data proxy registration msg does already verify that the payout address is valid.

Yeee, just saying we should have a log statement that says unreachable- more as a paranoia thing than anything else.

gluax · 2025-08-17T18:20:16Z

x/core/keeper/gas_meter.go

+			amount = math.MinInt(dist.ExecutorReward.Amount, remainingEscrow)
+			staker, err := k.GetStaker(ctx, dist.ExecutorReward.Identity)
+			if err != nil {
+				return err


The contract did not error in this case. IT would simply emit an error event and move on to the next iteration of the loop. The return statement would instead exit the function early not doing the payouts at all.

Or is this another unreachable situation due to earlier checks? Or can executors ever unregister so their proxy no longer exists?

TLDR this is a change from the contract.

gluax · 2025-08-17T18:23:34Z

x/core/keeper/gas_meter.go

+		}
+
+		remainingEscrow = remainingEscrow.Sub(amount)
+	}


We should be cautious with the errors in this loop. The contract would never error in this loop and instead would skip over the distrubtion and move on to the next one. The returns here, if I understand correctly, would cause this not to happen.

Though I think when we copy more of the tests over from the contract any issues here should be caught

gluax · 2025-08-17T18:42:20Z

x/core/keeper/msg_server_dr.go

+	drID, err := msg.MsgHash()
+	if err != nil {
+		return nil, err
+	}
+	exists, err := m.HasDataRequest(ctx, drID)


Difference from contract here- The hash function on contract returns a [u8; 32] and the map stores them by that not the hex string. Which I commented on the keeper why we would prefer this.

gluax · 2025-08-17T18:46:37Z

x/core/keeper/keeper.go

+	stakers collections.Map[string, types.Staker]
+
+	// Data request-related states:
+	// dataRequests is a map of data request IDs to data request objects.
+	dataRequests collections.Map[string, types.DataRequest]


So far looking at these, this is a change, we stored these by a enforced length of u8 arrays. The reason being this data structure under the hood convertes the Keys to a byte array anyways. So we are going from byte array -> hex string -> byte array again which is wasteful.

Maybe some of the other collcetions are using string keys where they should be byte arrays as well no sure. Going through as I go through.

Yeah I think you're right. Will try changing the data request ID to bytes

Sorry, I should have linked to it. Anywhere the key is used, it's as bytes. If you drill down this is done with a copy which is O(n) every time just like byte[]()(for a string)

gluax · 2025-08-17T18:48:36Z

x/core/keeper/msg_server.go

+	if err != nil {
+		return nil, err
+	}
+	publicKey, err := hex.DecodeString(msg.PublicKey)


We should be using the byte array version to check/update see the comment on the keeper to see the rationale there.

gluax · 2025-08-17T18:52:15Z

x/core/keeper/msg_server_dr.go

+		Escrow:            msg.Funds.Amount,
+		TimeoutHeight:     ctx.BlockHeight() + int64(drConfig.CommitTimeoutInBlocks),


I'm still unsure if these should be attached here or not. I think we can leave it for now but we should do the gas calculation stuff again to see if it improved/worsened things.

x/core/keeper/msg_server_dr.go

gluax

Good job HY!

x/core/keeper/msg_server_dr.go

gluax · 2025-08-20T20:41:00Z

x/core/keeper/msg_server_dr.go

+	// Verify against the stored commit.
+	expectedCommit, err := msg.RevealHash()
+	if err != nil {
+		return nil, err
+	}
+	if !bytes.Equal(commit, expectedCommit) {
+		return nil, types.ErrRevealMismatch
+	}


Let's move this check to before the msg.Valdiate call, checking this is cheaper than checking possibly a large amount of proxy public keys. So better to get this check out of the way first and fail the commit sooner than checking all of those then failing.

gluax · 2025-08-20T20:50:54Z

x/core/keeper/stakers.go

+func (k Keeper) GetStakersCount(ctx sdk.Context) (uint32, error) {
+	count := uint32(0)
+	err := k.stakers.Walk(ctx, nil, func(_ string, _ types.Staker) (stop bool, err error) {
+		count++
+		return false, nil
+	})
+	return count, err
+}


We should definitely modify the stakers on the keeper struct to have a count so we don't have to iterate it every time. It's okay-ish for while things are allowlisted, but definitely not for when we turn that off.

gluax · 2025-08-20T20:53:52Z

x/core/keeper/timeout_queue.go

+		dr, err := k.GetDataRequest(ctx, drID)
+		if err != nil {
+			return err
+		}
+
+		err = k.UpdateDataRequestIndexing(ctx, dr.Index(), dr.Status, types.DATA_REQUEST_STATUS_TALLYING)


I do think this is a drawback of storing the status on the Data Request itself. We have to get the data request as well everytime instead of just updating a Status map.

gluax · 2025-08-20T20:58:29Z

x/core/types/msgs.go

+	if !semver.IsValid("v"+m.Version) || semver.Prerelease("v"+m.Version) != "" || semver.Build("v"+m.Version) != "" {
+		return ErrInvalidVersion.Wrapf("%s", m.Version)
+	}


I don't remember if we need to do this in multiple places- but if we do could be a good ideal to pull this into a separate function and re-use it

gluax · 2025-08-20T21:11:39Z

x/core/types/params.go

+	return nil
+}
+
+func (dc *DataRequestConfig) Validate() error {


Ah can ingnore my earlier comment about NonZero stuff I hadn't seen this yet

…nd dispatching

- Also leaves out outdated integration test files for now: endblock_fuzz_test.go and most of endblock_test.go.

- Rename some field names for clarity. - Use methods instead of direct access to collections. - Improve comments. - Improve PostDataRequest message validation. Add relevant tests.

- Change block height types to int64, except for hashing - Lint

- Fixes a bug where status transition from committing to tallying was not considered as a possibility. - Committing, revealing, and tallying are combined into a single collection with different prefixes. - DataRequestStatus enum type now follows proto3 requirements. - Invalid status transition is now identified as an error in UpdateDataRequestIndexing. - DataRequest's TimeoutHeight is set to -1 if it is not in timeout queue.

- Stop distribution if data request escrow runs out. - RevealBody.Reveal is now a byte slice instead of a base64 string. - Core EndBlock now completes the full tally process of a given data request in one loop iteration. - Validate that a reveal's exit code can fit in a uint8. - Validate version in PostDataRequest. - Basic module parameter validation

hacheigriega · 2025-09-05T19:33:03Z

I will split up this PR and open new ones since this is getting way too long. I will keep this in draft and make sure to address @gluax 's comments.

hacheigriega force-pushed the hy/core branch 5 times, most recently from d797d07 to 06b662d Compare August 15, 2025 01:11

hacheigriega requested review from Thomasvdam and gluax August 15, 2025 01:34

hacheigriega force-pushed the hy/core branch 2 times, most recently from 444f094 to 1716ec1 Compare August 15, 2025 13:50

hacheigriega changed the base branch from main to dev August 15, 2025 14:18

hacheigriega commented Aug 15, 2025

View reviewed changes

x/core/types/abci_types.go Outdated Show resolved Hide resolved

gluax reviewed Aug 15, 2025

View reviewed changes

hacheigriega force-pushed the hy/core branch 3 times, most recently from 59f7878 to 206282d Compare August 16, 2025 12:56

gluax reviewed Aug 17, 2025

View reviewed changes

hacheigriega force-pushed the hy/core branch 2 times, most recently from 240a2a7 to 7f9067a Compare August 18, 2025 15:29

gluax reviewed Aug 20, 2025

View reviewed changes

hacheigriega added 8 commits September 3, 2025 17:05

feat(x/core): skeleton code for core module

6bb8623

feat(x/wasm): x/wasm wrapper to redirect Core Contract execute msgs

8c4d623

feat(x/core): Stakers map

d09e805

feat(x/core): Stake transaction

899018c

feat(x/core): PostDataRequest transaction

a75e20c

feat(x/core): Commit tx except fee refund

acbea82

feat(x/wasm,x/core): Reveal tx with Core Contract msg shim encoding a…

a1f24bc

…nd dispatching

refactor(x/core): move x/tally to x/core and remove upgrade handlers

9a081d1

- Also leaves out outdated integration test files for now: endblock_fuzz_test.go and most of endblock_test.go.

hacheigriega marked this pull request as draft September 4, 2025 15:57

feat(x/core): ExpireDataRequests with full DR flow integration testing

3e9a432

hacheigriega added 7 commits September 4, 2025 17:11

test(x/core): benchmark testing of data request flow

2f39d48

feat(x/core): gas meter payouts and removing data requests after tally

6095771

chore(x/core): cleanup of x/core

5db8ec4

- Rename some field names for clarity. - Use methods instead of direct access to collections. - Improve comments. - Improve PostDataRequest message validation. Add relevant tests.

chore(x/core): lint and some type changes to reduce casting

a338235

- Change block height types to int64, except for hashing - Lint

fix: refund remaining escrow instead of original escrow

d4e6efc

hacheigriega force-pushed the hy/core branch from 004b29f to d4e6efc Compare September 4, 2025 21:53

hacheigriega changed the base branch from dev to main September 5, 2025 13:26

		Escrow: msg.Funds.Amount,
		TimeoutHeight: ctx.BlockHeight() + int64(drConfig.CommitTimeoutInBlocks),

x/core with core features #585

Are you sure you want to change the base?

x/core with core features #585

Uh oh!

Conversation

hacheigriega commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation of Changes

States

Messages

Testing

Dependency Chain

Related PRs and Issues

Uh oh!

hacheigriega commented Aug 15, 2025

Uh oh!

Uh oh!

gluax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gluax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gluax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

hacheigriega commented Aug 13, 2025 •

edited

Loading