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

Tracking rewards #4863

Closed
piersy opened this issue Aug 16, 2023 · 7 comments
Closed

Tracking rewards #4863

piersy opened this issue Aug 16, 2023 · 7 comments

Comments

@piersy
Copy link
Contributor

piersy commented Aug 16, 2023

Description

Rewards are generated in response to work done by a smesher, the system then sends the rewards to the coinbase (account) specified by that smesher.

Although the smesher id that was responsible for generating a reward is available at the time a reward is generated we do not currently store that in our database.

The current schema is:

    coinbase     CHAR(24),
    layer        INT NOT NULL,
    total_reward UNSIGNED LONG INT,
    layer_reward UNSIGNED LONG INT,
    PRIMARY KEY (coinbase, layer)

Smeshers are free to select their coinbase and also free to change their coinbase at each epoch. This means that there can be many smeshers sharing the same coinbase and also that over time a single smesher can be associated with multiple coinbase addresses.

Currently it seems there are users that are running multiple smeshers with a single coinbase and they are not able to discover how rewards were allocated between their smeshers, only the total rewards that all smeshers earned.

It seems like we would want to be able to retrieve rewards by smesher id, coinbase or both.

Affected code

The storage and retrieval of rewards ocurrs at sql/rewards/rewards.go

This issue appears in commit hash: c7a7468

Approach

@countvonzero suggested factoring out the purely functional parts of the VM's execution so that we could re run all blocks through there and use the results to populate a new rewards table without the possibility of accidentally mutating existing database state. Then at some later point we could switch the main code from the old table to the new table. Performing this work without mutating existing tables also provides the option to rollback if we encounter any unexpected issues.

@lrettig
Copy link
Member

lrettig commented Aug 16, 2023

It seems like simply adding a smesher_id column to the rewards table would be sufficient, unless I'm missing something. And it should be possible to deduce which smesher is responsible for which reward at the time rewards are calculated, since they're based on ATXs/eligibilities which contain smesher IDs.

@piersy
Copy link
Contributor Author

piersy commented Aug 17, 2023

@lrettig #4850 modifies the API responses for AccountDataStream and GlobalStateStream to include the smesher ID, it's a fairly simple modification.

What you suggested sounds like it would work, I don't see a reason for doing anything else.

@piersy piersy assigned piersy and unassigned piersy Aug 17, 2023
@brunovale91
Copy link

brunovale91 commented Aug 27, 2023

I'm running a modified version of the node where smesher id is populated from getting the ATX header on global state account data.

So far the only way I found to get the ATX id is to match it against the block rewards (anyReward). So I need to iterate over the layer blocks to find the right ATX id and fetch the header. However I need to match coinbase between the account rewards and the block rewards which can lead to incorrect data if there are multiple rewards in the same layer for the same coinbase. Also this approach doesn't seem to be efficient. Maybe there's a better way to populate it but not sure.

func getSmesherId(s GlobalStateService, r *types.Reward) ([]byte, error) {
	layer, _ := s.mesh.GetLayer(r.Layer)
	var smesherId []byte
	for _, b := range layer.Blocks() {
		for _, br := range b.Rewards {
			atxHdr, err := s.cdb.GetAtxHeader(br.AtxID)
			if err != nil {
				s.logger.With().Error("could not get atx header", br.AtxID, log.Err(err))
				return nil, status.Errorf(codes.Internal, "error reading atx header")
			}
			if atxHdr.Coinbase.String() == r.Coinbase.String() {
				smesherId = atxHdr.NodeID.Bytes()
				break
			}

		}
	}
	return smesherId, nil
}

It would be ideal to add the ATX id to the rewards table since it would allow to match the ATX with the reward and then get the smesher ID.

Also the call to get all rewards seems to be very slow at the time.

From what I've seen the method to get the list of rewards is not paginated even though the GRPC API supports offset and limit.

It would probably be better to make two queries, one to get the totalCount (which should use the index) and another one to fetch only the required page.

@dshulyak
Copy link
Contributor

dshulyak commented Oct 20, 2023

to summarize what was shared in this issue:

there a missing information in API. and there is a change that fixes that problem #4850 . explorer will be missing data with smesher id if explorer loses stream for any reason (reason can be a temporary network problem, or restart of the explorer itself). and it will be possible to fix it only by resyncing a spacemesh node that is used as backend for explorer. i think this is not sufficient and will create confusion, as feature will some time work and some time not.

to fix this problem we want to extend db schema with smesher id and start writing smesher id to the database together with reward object. the non-trivial part of this problem is what we do about old records. one option is to require resync if this data is important for your use case. obviously we will resync public explorer, annd everyone who uses that will see new data.

another option is to rerun block execution, which is less trivial and requires testing. but will not require resync.

@brunovale91
Copy link

to summarize what was shared in this issue:

there a missing information in API. and there is a change that fixes that problem #4850 . explorer will be missing data with smesher id if explorer loses stream for any reason (reason can be a temporary network problem, or restart of the explorer itself). and it will be possible to fix it only by resyncing a spacemesh node that is used as backend for explorer. i think this is not sufficient and will create confusion, as feature will some time work and some time not.

to fix this problem we want to extend db schema with smesher id and start writing smesher id to the database together with reward object. the non-trivial part of this problem is what we do about old records. one option is to require resync if this data is important for your use case. obviously we will resync public explorer, annd everyone who uses that will see new data.

another option is to rerun block execution, which is less trivial and requires testing. but will not require resync.

This is 100% true, one option would be to have a special version of the node with some event stream service. This decouples the node from the events and it makes it much easier to reindex the events on the explorer BE. I’ve done that here:
Node with Nats

Bare in mind this is a version for my own use in the app and I’m new to go 😆

Sink part here:
https://github.com/swarmbit/spacemesh-state-api

This has been working fine for the app for a few weeks now and I’ve already had to reindex the mongo db a few times (without touching the node)

@brunovale91
Copy link

https://github.com/swarmbit/go-spacemesh/tree/v1.2.1-REWARDS-TABLE

Version changing rewards table without migration of old data.

@lrettig
Copy link
Member

lrettig commented Oct 25, 2023

the non-trivial part of this problem is what we do about old records. one option is to require resync if this data is important for your use case. obviously we will resync public explorer, annd everyone who uses that will see new data.

another option is to rerun block execution, which is less trivial and requires testing. but will not require resync.

I think the former is perfectly acceptable for now. Users who care can resync to populate this data historically. Most users don't care. And as you said we'll obviously resync for explorer use case.

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

Successfully merging a pull request may close this issue.

4 participants