[Merged by Bors] - vm, api: add smesherID to rewards #5199

lrettig · 2023-10-25T20:20:30Z

Motivation

Based on #4850

Closes #4529
Closes #4850
Closes #4863
Closes #4964
Closes #5183

Changes

Refactors rewards table. Adds smesherID column, migrates data. Does not update old data (smesherID will be NULL for old data, unless resynced).
Updates database queries.
Returns smesherID in all API queries

Migrates existing database table. All future database writes will include smesherID. We do not update old data, which will simply have missing smesherID data. This is fine for most use cases.

primary key of rewards table has changed: it used to be (coinbase, layer) but is now (smesherID, layer). rewards must thus have distinct smesherID values.

codecov · 2023-11-08T22:22:31Z

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (3ae2bf3) 77.5% compared to head (190ac14) 77.5%.

Files	Patch %	Lines
sql/rewards/rewards.go	82.0%	8 Missing and 1 partial ⚠️
mesh/mesh.go	0.0%	4 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           develop   #5199   +/-   ##
=======================================
  Coverage     77.5%   77.5%           
=======================================
  Files          253     252    -1     
  Lines        29641   29650    +9     
=======================================
+ Hits         22972   22983   +11     
- Misses        5208    5209    +1     
+ Partials      1461    1458    -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

genvm/vm_test.go

sql/migrations/state/0007_next.sql

sql/rewards/rewards.go

genvm/vm.go

sql/rewards/rewards.go

cmd/bootstrapper/generator_test.go

In tests, rather than using rand Fix changelog

Drop "WITHOUT ROWID" to prevent NOT NULL error on new primary key column.

lrettig · 2023-11-16T10:39:23Z

@fasmat @dshulyak please take a look at the latest commit: 1595ea9

I had to remove the WITHOUT ROWID from the new rewards table definition to allow the migration of the old data, since it obviously doesn't have pubkey data (and this column is part of the primary key). This is a constraint on sqlite. Do we have concerns that this might impact performance?

Lost in merge

Adds one more migration

To prevent import cycle in tests

sql/database.go

fasmat · 2023-11-23T09:40:44Z

@fasmat @dshulyak please take a look at the latest commit: 1595ea9

I had to remove the WITHOUT ROWID from the new rewards table definition to allow the migration of the old data, since it obviously doesn't have pubkey data (and this column is part of the primary key). This is a constraint on sqlite. Do we have concerns that this might impact performance?

I think this will add 8 bytes per row that are persisted in the DB but not really needed even at ~ 1 mio entries we have for that table that's probably negligible since the PR also adds 32 bytes for the smesher ID. I don't think queries are slower with a Row ID, so I'd be OK with the change.

dshulyak · 2023-11-27T11:32:13Z

what it does is that you have to query 2 b-trees instead of one. so if you have small unique key it is always better to have one b-tree. i can't tell without measuring, but generally single tree will be much better for range queries that hit that exact primary index (for example all rewards from smesher X, or all rewards inbetween from smesher X inbeetween layers Y and Z)

lrettig · 2023-11-27T20:02:58Z

The only other option here is to force a resync from scratch, or to rerun all transactions through the VM to reconstruct the missing data. Even if we merge this PR as-is, those options are still on the table for the future - that would be my preference.

dshulyak · 2023-12-01T13:14:28Z

yes i don't mind looking into any perf issues later

@mafa

Final review feedback, h/t @mafa

lrettig · 2023-12-02T00:48:08Z

bors merge

## Motivation See #4863 Based on #4850 Closes #4529 Closes #4850 Closes #4863 Closes #4964 Closes #5183 ## Changes - Refactors `rewards` table. Adds smesherID column, migrates data. Does not update old data (smesherID will be NULL for old data, unless resynced). - Updates database queries. - Returns smesherID in all API queries Co-authored-by: Piers Powlesland <pierspowlesland@gmail.com> Co-authored-by: piersy <pierspowlesland@gmail.com> Co-authored-by: Dmitry Shulyak <yashulyak@gmail.com>

spacemesh-bors · 2023-12-02T01:37:33Z

Pull request successfully merged into develop.

Build succeeded:

piersy and others added 18 commits August 15, 2023 15:13

Add smesher ID to AccountDataStream and GlobalStateStream responses

0008bfc

Update tests to check for smesherID

fed60d1

Fix mesh tests

83a8ebe

Merge branch 'develop' into piersy/add_smesher_id_to_grpc_reward_object

40cc403

Merge branch 'develop' into piersy/add_smesher_id_to_grpc_reward_object

47f701b

Merge branch 'develop' into piersy/add_smesher_id_to_grpc_reward_object

7179380

Merge branch 'develop' into piersy/add_smesher_id_to_grpc_reward_object

7890737

Merge branch 'develop' into piersy/add_smesher_id_to_grpc_reward_object

9163697

Add smesherID to rewards in database table

d0712ec

Migrates existing database table. All future database writes will include smesherID. We do not update old data, which will simply have missing smesherID data. This is fine for most use cases.

Fix queries

eeecbca

Fix tests

980cec0

Fix migration

e949b10

go generate

6458012

Merge branch 'develop' into fix_rewards

56cc2be

Fix tests

dc434a9

Fix rewards tests

e6f356d

primary key of rewards table has changed: it used to be (coinbase, layer) but is now (smesherID, layer). rewards must thus have distinct smesherID values.

Fix rewards tests

ef3091d

Add tests for ListBySmesherId

c55d08b

lrettig added 2 commits November 9, 2023 14:08

Merge branch 'develop' into fix_rewards

1093e27

Add error checks

d38db5d

lrettig marked this pull request as ready for review November 9, 2023 20:38

lrettig requested review from dshulyak, fasmat and poszu as code owners November 9, 2023 20:38

dshulyak reviewed Nov 10, 2023

View reviewed changes

genvm/vm_test.go Outdated Show resolved Hide resolved

sql/migrations/state/0007_next.sql Outdated Show resolved Hide resolved

sql/rewards/rewards.go Show resolved Hide resolved

genvm/vm.go Outdated Show resolved Hide resolved

sql/rewards/rewards.go Outdated Show resolved Hide resolved

fasmat reviewed Nov 10, 2023

View reviewed changes

cmd/bootstrapper/generator_test.go Outdated Show resolved Hide resolved

lrettig added 3 commits November 10, 2023 13:01

Merge branch 'develop' into fix_rewards

95e948d

Update changelog

682777e

Derive coinbase from address deterministically

fab0743

In tests, rather than using rand Fix changelog

More linting

1209a9d

fasmat approved these changes Nov 13, 2023

View reviewed changes

dshulyak changed the title ~~Add smesherID to rewards~~ vm, api: add smesherID to rewards Nov 14, 2023

dshulyak approved these changes Nov 14, 2023

View reviewed changes

lrettig added 2 commits November 16, 2023 05:09

Merge branch 'develop' into fix_rewards

35094c4

Fix sqlite migration of rewards table

1595ea9

Drop "WITHOUT ROWID" to prevent NOT NULL error on new primary key column.

lrettig added 7 commits November 16, 2023 05:58

Restore migration file

018d66f

Lost in merge

Fix migrations unit test

f209784

Adds one more migration

Merge branch 'develop' into fix_rewards

45df14a

Add tests WIP

976d3e8

Merge branch 'develop' into fix_rewards

b506903

Refactor rewards/migrations test

2cb8825

To prevent import cycle in tests

Finish rewards/migration test

f36167e

lrettig requested review from dshulyak and fasmat November 22, 2023 21:08

Properly check all errors in test

3c58ff7

fasmat reviewed Nov 23, 2023

View reviewed changes

sql/database.go Outdated Show resolved Hide resolved

lrettig added 2 commits December 1, 2023 16:08

Merge branch 'develop' into fix_rewards

ad88922

Remove useless loop

190ac14

Final review feedback, h/t @mafa

spacemesh-bors bot changed the title ~~vm, api: add smesherID to rewards~~ [Merged by Bors] - vm, api: add smesherID to rewards Dec 2, 2023

spacemesh-bors bot closed this Dec 2, 2023

spacemesh-bors bot deleted the fix_rewards branch December 2, 2023 01:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - vm, api: add smesherID to rewards #5199

[Merged by Bors] - vm, api: add smesherID to rewards #5199

lrettig commented Oct 25, 2023 •

edited by dshulyak

codecov bot commented Nov 8, 2023 •

edited

lrettig commented Nov 16, 2023

fasmat commented Nov 23, 2023

dshulyak commented Nov 27, 2023

lrettig commented Nov 27, 2023

dshulyak commented Dec 1, 2023

lrettig commented Dec 2, 2023

spacemesh-bors bot commented Dec 2, 2023

[Merged by Bors] - vm, api: add smesherID to rewards #5199

[Merged by Bors] - vm, api: add smesherID to rewards #5199

Conversation

lrettig commented Oct 25, 2023 • edited by dshulyak

Motivation

Changes

codecov bot commented Nov 8, 2023 • edited

Codecov Report

lrettig commented Nov 16, 2023

fasmat commented Nov 23, 2023

dshulyak commented Nov 27, 2023

lrettig commented Nov 27, 2023

dshulyak commented Dec 1, 2023

lrettig commented Dec 2, 2023

spacemesh-bors bot commented Dec 2, 2023

lrettig commented Oct 25, 2023 •

edited by dshulyak

codecov bot commented Nov 8, 2023 •

edited