epoching: keeper functions, queries, and testing infra #19

SebastianElvis · 2022-06-21T07:17:54Z

This PR introduces keeper functions, queries, and test infra for the epoching module.

TODOs (will be done in subsequent PRs):

Unit/fuzz tests, depending on properties we want to test

SebastianElvis · 2022-06-22T05:17:43Z

After this PR and #18, the next task would be state transition upon BeginBlock and EndBlock. Implementing these state transitions require our proposed modifications in the Cosmos SDK. Maybe after the two PRs, it will be the right time to fork and work on Cosmos SDK.

aakoshh · 2022-06-22T08:02:06Z

Maybe after the two PRs, it will be the right time to fork and work on Cosmos SDK.

Frojdi suggested to @vitsalis that we try to do it without forking the SDK, by just copy pasting the necessary lines into our codebase and only calling ApplyAndReturnValidatorSetUpdates in our EndBlock. The idea was that if somebody would want to develop against Babylon as a library Go might mix up forked and non-forked stuff.

SebastianElvis · 2022-06-22T08:08:23Z

Maybe after the two PRs, it will be the right time to fork and work on Cosmos SDK.

Frojdi suggested to @vitsalis that we try to do it without forking the SDK, by just copy pasting the necessary lines into our codebase and only calling ApplyAndReturnValidatorSetUpdates in our EndBlock. The idea was that if somebody would want to develop against Babylon as a library Go might mix up forked and non-forked stuff.

Right, so basically we need to do the following:

Exclude the staking module from ModuleManager's EndBlocker/BeginBlocker function
Copy/paste necessary functions to the epoching module and make our modifications
Execute the modified logic in the epoching module totally, and invoke the staking module for non-modified logic

I will follow this approach then. Thanks for the information!

aakoshh · 2022-06-22T08:13:01Z

Yes, that's exactly right. If you agree with this approach, it's probably not a big risk to keep those lines in sync (while it only calls public methods). Frojdi thinks this part doesn't change often.

aakoshh

Great!

Good idea using the count for providing a unique ID. A potential alternative could be using some sort of a timestamp like in https://github.com/cosmos/cosmos-sdk/blob/a2761bdc6f23500a2b38d398ee2a423f2a5311c0/x/staking/keeper/delegation.go#L374, but they seem to store lists under the keys, so it's actually a bit more complicated.

I don't really follow the epoch boundary logic, though.

It would be worth following up with a PR with fuzz tests to check that the invariants are maintained.

aakoshh · 2022-06-22T08:36:12Z

x/epoching/keeper/grpc_query.go

+ epochNumber, err := k.GetEpochNumber(ctx)
+ if err != nil {
+ return nil, err
+ }
+ epochBoundary, err := k.GetEpochBoundary(ctx)


I wonder if there is any race condition here with block processing, or does the SDK take care of providing a consistent view throughout the query.

IIUC Cosmos SDK has taken care of this. The module manager triggers BeginBlock and EndBlock sequentially for each module. At any time slot there will only be one module processing ctx, which includes blocks.

Yes, but these are gRPC queries which can run any time by anyone who accesses the RPC endpoints. It's probably okay, but it would be nice to know for sure.

You are right, will look into this and get back to you here tomorrow

I'm not fully certain, but there seems to be no handling on race conditions caused by concurrent GPRC requests. Cosmos SDK's GPRC server is implemented here https://github.com/cosmos/cosmos-sdk/blob/v0.45.5/server/grpc/grpc_web.go#L15-L45. It basically uses Go's net/http library. Upon each GPRC query, it creates a goroutine to handle it.

In this case, it seems tricky if we want to make multiple sequential DB read/write upon a single query..

aakoshh · 2022-06-22T08:41:45Z

x/epoching/keeper/grpc_query.go

 func (k Keeper) EpochMsgs(c context.Context, req *types.QueryEpochMsgsRequest) (*types.QueryEpochMsgsResponse, error) {
- panic("TODO: unimplemented")
+ ctx := sdk.UnwrapSDKContext(c)
+ msgs, err := k.GetEpochMsgs(ctx)
+ if err != nil {
+ return nil, err
+ }
+ resp := &types.QueryEpochMsgsResponse{
+ Msgs: msgs,
+ Pagination: &qtypes.PageResponse{
+ Total: uint64(len(msgs)),
+ },
+ }
+ return resp, nil


What seems to be missing here is actual pagination handling, you always return all messages. This might violate the API contract by ignoring any Limit in the request. Actually it even has a flag CountTotal to indicate whether Total should be calculated, as well as Reverse.

Here's an example of using this, it looks like the SDK takes care of the heavy lifting: https://github.com/cosmos/cosmos-sdk/blob/v0.45.5/x/staking/keeper/grpc_query.go#L156-L174

Thanks! Will do.

aakoshh · 2022-06-22T08:48:44Z

x/epoching/keeper/grpc_query_test.go

+func TestParamsQuery(t *testing.T) {
+ keeper, ctx := testkeeper.EpochingKeeper(t)
+ wctx := sdk.WrapSDKContext(ctx)
+ params := types.DefaultParams()


It's not a big deal, but it's worth using fuzz tests even for this sort of thing. When I see a test inserting and asserting something like DefaultParams I am always wary whether something like this might be going on, showing a false positive by returning DefaultParams from the keeper if it can't find a record for example. Testing with non-default input is always more convincing, and using fuzzing can remove the burden of coming up with hard coded values.

Agree. Perhaps we can have the fuzz tests in a separate PR? since the tests seem to heavily depend on the APIs and the properties we want to test.

Sure, no problem. Luckily you will still be able to use almost the same test, as the data is an in-memory one, so you can just create a new one in each test, like it's done now. The only difference is that instead of params := types.DefaultParams() you could use one of the libraries I linked to to pupulate the params with random values.

aakoshh · 2022-06-22T08:51:23Z

x/epoching/keeper/keeper.go

+ epochInterval := sdk.NewUint(k.GetParams(ctx).EpochInterval)
+
+ // hits epoch boundary
+ existingBlocks := epochNumber.Mod(epochInterval)


Shouldn't this be Mul? Aren't we dealing with things like "we are in epoch 5, with the interval being 1000 blocks"?

Ah you are right. I messed up with the epoch number and the height. Will address this function.

aakoshh · 2022-06-22T08:52:20Z

x/epoching/keeper/keeper.go

+ // hits epoch boundary
+ existingBlocks := epochNumber.Mod(epochInterval)
+ if existingBlocks.IsZero() {
+ return epochNumber, nil


How could the epoch number be a correct block height for the end?

x/epoching/keeper/keeper.go

aakoshh · 2022-06-22T08:55:49Z

x/epoching/keeper/keeper.go

+
+ bz := store.Get(types.QueueLengthKey)
+ if bz == nil {
+ return sdk.NewUint(uint64(DefaultQueueLength)), nil


What possible default value other than 0 could this take?

I cannot think of any other value. Perhaps this is not a "default" value: the queue has no item if no msg is queued. Will consider hard-code 0 here rather than having a new constant DefaultQueueLength

aakoshh · 2022-06-22T08:56:25Z

x/epoching/keeper/keeper.go

+ return err
+ }
+
+ store.Set(types.EpochNumberKey, queueLenBytes)


Suggested change

store.Set(types.EpochNumberKey, queueLenBytes)

store.Set(types.QueueLengthKey, queueLenBytes)

aakoshh · 2022-06-22T08:57:04Z

x/epoching/keeper/keeper.go

+}
+
+// IncQueueLength adds the queue length by 1
+func (k Keeper) IncQueueLength(ctx sdk.Context) error {


Does this need to be public? Seems like the only time it should be called is during EnqueueMsg.

aakoshh · 2022-06-22T08:59:41Z

x/epoching/keeper/keeper.go

+ defer iterator.Close()
+ for ; iterator.Valid(); iterator.Next() {
+ key := iterator.Key()
+ queuedMsgBytes := store.Get(key)


There should be something like iterator.Value(), look, so you don't have to do a separate lookup.

SebastianElvis · 2022-06-23T10:30:50Z

Thanks for the comments Akosh! I have addressed the comments and investigated the GPRC server stuff (replied above). Feel free to review again.

aakoshh · 2022-06-23T12:29:41Z

x/epoching/keeper/grpc_query.go

+ // get value in bytes
+ storeValue := store.Get(storeKey)


Why not use the value []byte parameter passed to this func?

aakoshh · 2022-06-23T12:41:37Z

x/epoching/keeper/grpc_query.go

+// EpochMsgs handles the QueryEpochMsgsRequest query
 func (k Keeper) EpochMsgs(c context.Context, req *types.QueryEpochMsgsRequest) (*types.QueryEpochMsgsResponse, error) {


One thing to keep in mind with this query is that the epoch might end between pagination requests, so it might be presenting an inconsistent result by the time someone gets to the end.

There are a few fixes that I can think of, depending on what we want to achieve.

We could add the epoch number to the query, and return nothing if the current epoch number is different. But it's a bit of pain to have to set it and not know why there's no result.

We could not reset the key to 0 when the queue is cleared, and just keep incrementing the ID forever. That way when the next query comes, it might skip some records that have been deleted, then resume from the next available record which has a higher key than the value in the pagination data structure.

I suppose we can do nothing, in which case some records that have been inserted after the delete might be skipped because their keys are lower than the pagionation state.

aakoshh

Nice! I don't see any blockers, but if possible it would be good to save that extra lookup during querying and just use value. That's what the other iterator in the keeper is doing as well.

SebastianElvis · 2022-06-24T01:04:47Z

Thanks! Have fixed the extra lookup and added TODOs for the concurrency issue in EpochMsgs, and merged this PR.

Will have some subsequent PRs to

add fuzz/unit tests
fix race condition issues (I will need to look into how other Cosmos-SDK-based projects handle such issues.)

SebastianElvis added 5 commits June 21, 2022 16:41

init

95b3eac

init

da7b642

minor

b7c63bb

Merge branch 'main' into epoching-keeper-functions

b14b780

epoching test stuff

8c10a37

SebastianElvis changed the title ~~epoching: keeper functions~~ epoching: keeper functions and testing infra Jun 22, 2022

SebastianElvis added 2 commits June 22, 2022 14:59

minor

c8fe330

minor fix and add comments

eae24a3

SebastianElvis marked this pull request as ready for review June 22, 2022 05:12

SebastianElvis requested a review from aakoshh June 22, 2022 05:13

query impl

55ebce3

SebastianElvis changed the title ~~epoching: keeper functions and testing infra~~ epoching: keeper functions, queries, and testing infra Jun 22, 2022

aakoshh reviewed Jun 22, 2022

View reviewed changes

SebastianElvis added 2 commits June 23, 2022 14:39

fix

98caf7b

fix

a834646

SebastianElvis requested a review from aakoshh June 23, 2022 10:30

aakoshh reviewed Jun 23, 2022

View reviewed changes

aakoshh approved these changes Jun 23, 2022

View reviewed changes

fix

e3c029e

SebastianElvis merged commit aa07c40 into main Jun 24, 2022

SebastianElvis deleted the epoching-keeper-functions branch June 24, 2022 01:04

KonradStaniec pushed a commit that referenced this pull request Oct 25, 2023

BTC staking / finality: CLIs for submitting txs and e2e commands (#19)

067082b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

epoching: keeper functions, queries, and testing infra #19

epoching: keeper functions, queries, and testing infra #19

SebastianElvis commented Jun 21, 2022 •

edited

Loading

SebastianElvis commented Jun 22, 2022

aakoshh commented Jun 22, 2022

SebastianElvis commented Jun 22, 2022

aakoshh commented Jun 22, 2022

aakoshh left a comment

aakoshh Jun 22, 2022

SebastianElvis Jun 22, 2022

aakoshh Jun 22, 2022

SebastianElvis Jun 22, 2022

SebastianElvis Jun 23, 2022

aakoshh Jun 22, 2022

SebastianElvis Jun 22, 2022

aakoshh Jun 22, 2022

SebastianElvis Jun 22, 2022

aakoshh Jun 22, 2022

aakoshh Jun 22, 2022

SebastianElvis Jun 22, 2022

aakoshh Jun 22, 2022

SebastianElvis Jun 22, 2022

aakoshh Jun 22, 2022

SebastianElvis Jun 22, 2022

aakoshh Jun 22, 2022

aakoshh Jun 22, 2022

aakoshh Jun 22, 2022

SebastianElvis commented Jun 23, 2022

aakoshh Jun 23, 2022

aakoshh Jun 23, 2022

aakoshh left a comment

SebastianElvis commented Jun 24, 2022

	store.Set(types.EpochNumberKey, queueLenBytes)
	store.Set(types.QueueLengthKey, queueLenBytes)

		// EpochMsgs handles the QueryEpochMsgsRequest query
		func (k Keeper) EpochMsgs(c context.Context, req types.QueryEpochMsgsRequest) (types.QueryEpochMsgsResponse, error) {

epoching: keeper functions, queries, and testing infra #19

epoching: keeper functions, queries, and testing infra #19

Conversation

SebastianElvis commented Jun 21, 2022 • edited Loading

SebastianElvis commented Jun 22, 2022

aakoshh commented Jun 22, 2022

SebastianElvis commented Jun 22, 2022

aakoshh commented Jun 22, 2022

aakoshh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianElvis commented Jun 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aakoshh left a comment

Choose a reason for hiding this comment

SebastianElvis commented Jun 24, 2022

SebastianElvis commented Jun 21, 2022 •

edited

Loading