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

feat(streamer): distribute rewards immediately in the current block #1173

Merged
merged 31 commits into from
Sep 20, 2024

Conversation

keruch
Copy link
Contributor

@keruch keruch commented Sep 2, 2024

Description

https://www.notion.so/dymension/ADR-x-Incentives-DoS-47238d8a29734ee686e25caef8b68c4f

Closes #XXX

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.

PR review checkboxes:

I have...

  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Targeted PR against the correct branch
  • included the correct type prefix in the PR title
  • Linked to the GitHub issue with discussion and accepted design
  • Targets only one GitHub issue
  • Wrote unit and integration tests
  • Wrote relevant migration scripts if necessary
  • All CI checks have passed
  • Added relevant godoc comments
  • Updated the scripts for local run, e.g genesis_config_commands.sh if the PR changes parameters
  • Add an issue in the e2e-tests repo if necessary

SDK Checklist

  • Import/Export Genesis
  • Registered Invariants
  • Registered Events
  • Updated openapi.yaml
  • No usage of go map
  • No usage of time.Now()
  • Used fixed point arithmetic and not float arithmetic
  • Avoid panicking in Begin/End block as much as possible
  • No unexpected math Overflow
  • Used sendCoin and not SendCoins
  • Out-of-block compute is bounded
  • No serialized ID at the end of store keys
  • UInt to byte conversion should use BigEndian

Full security checklist here


For Reviewer:

  • Confirmed the correct type prefix in the PR title
  • Reviewers assigned
  • Confirmed all author checklist items have been addressed

After reviewer approval:

  • In case the PR targets the main branch, PR should not be squash merge in order to keep meaningful git history.
  • In case the PR targets a release branch, PR must be rebased.

@keruch keruch self-assigned this Sep 2, 2024
@keruch keruch force-pushed the kirill/354-streamer-distributes-immediately branch from a335aee to a33cab4 Compare September 2, 2024 17:40
x/streamer/keeper/abci.go Fixed Show fixed Hide fixed
x/streamer/keeper/distribute.go Fixed Show resolved Hide resolved
@keruch keruch force-pushed the kirill/1010-streamer-pagination branch from 25caeb2 to a1e983b Compare September 3, 2024 08:37
@keruch keruch force-pushed the kirill/354-streamer-distributes-immediately branch from a8717ef to a939117 Compare September 3, 2024 16:48
@keruch keruch marked this pull request as ready for review September 3, 2024 16:48
@keruch keruch requested a review from a team as a code owner September 3, 2024 16:48
x/streamer/keeper/abci.go Fixed Show fixed Hide fixed
Copy link
Contributor

@zale144 zale144 left a comment

Choose a reason for hiding this comment

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

so, I can see that the logic in both EndBlock and AfterEpochEnd roughly has some similarities:

1. GetActiveStreams
2. CalculateRewards
3. SendCoinsFromModuleToModule
4. ik.Distribute

is there a way to somehow extract that logic into a common method?
the main difference I see is that EndBlock iterates over all epochPointers while AfterEpochEnd distributes for the current epochPointer

the is that the implementation is a bit convoluted, it's not trivial to follow the logic, so maybe it would help to centralize those critical pieces of logic

x/incentives/keeper/distribute.go Show resolved Hide resolved
x/streamer/keeper/hooks.go Outdated Show resolved Hide resolved
x/streamer/keeper/hooks.go Outdated Show resolved Hide resolved
x/streamer/keeper/cache.go Outdated Show resolved Hide resolved
x/streamer/keeper/cache.go Outdated Show resolved Hide resolved
x/streamer/keeper/cache.go Outdated Show resolved Hide resolved
x/streamer/keeper/cache.go Outdated Show resolved Hide resolved
x/streamer/keeper/cache.go Outdated Show resolved Hide resolved
x/streamer/keeper/hooks.go Outdated Show resolved Hide resolved
x/streamer/keeper/abci.go Outdated Show resolved Hide resolved
x/streamer/keeper/abci.go Fixed Show fixed Hide fixed
x/streamer/keeper/abci.go Fixed Show fixed Hide fixed
@keruch keruch requested a review from zale144 September 9, 2024 18:20
utils/cache/ordered.go Outdated Show resolved Hide resolved
utils/cache/ordered.go Show resolved Hide resolved
x/streamer/keeper/distribute.go Outdated Show resolved Hide resolved
x/streamer/keeper/distribute.go Outdated Show resolved Hide resolved
Base automatically changed from kirill/1010-streamer-pagination to main September 10, 2024 18:34
Copy link

codecov bot commented Sep 16, 2024

Codecov Report

Attention: Patch coverage is 75.37994% with 81 lines in your changes missing coverage. Please review.

Project coverage is 24.95%. Comparing base (031ecad) to head (90d79e3).
Report is 24 commits behind head on main.

Files with missing lines Patch % Lines
x/streamer/keeper/distribute.go 64.88% 36 Missing and 10 partials ⚠️
utils/cache/ordered.go 71.64% 19 Missing ⚠️
x/incentives/keeper/distribute.go 76.92% 4 Missing and 2 partials ⚠️
x/streamer/types/stream.pb.go 66.66% 3 Missing and 1 partial ⚠️
utils/cache/linked_list.go 94.87% 1 Missing and 1 partial ⚠️
x/streamer/keeper/abci.go 71.42% 1 Missing and 1 partial ⚠️
x/streamer/keeper/hooks.go 85.71% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1173      +/-   ##
==========================================
- Coverage   25.30%   24.95%   -0.36%     
==========================================
  Files         499      513      +14     
  Lines       98751   102755    +4004     
==========================================
+ Hits        24989    25642     +653     
- Misses      70523    73786    +3263     
- Partials     3239     3327      +88     

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

Copy link
Contributor

@danwt danwt left a comment

Choose a reason for hiding this comment

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

I will continue the review but it's better I ask questions first, mainly I don't quite understand the intent behind the linked list and its wrapper

https://github.com/dymensionxyz/dymension/pull/1173/files

Somehow I can't see the forest through the trees when it comes to the data structures and it's usage

  • please docstring all public methods on InsertionOrdered
  • in general better documentation of insertion ordered data structure
  • why need key func in insertion ordered?
  • no need for insertion ordered upsert multiple and upsert, just have upsert take variadic
  • Linked lists are not performant AT ALL afaiu, use a more hardware friendly data structure? does perf matter?
  • why linked list? what property there is important?
  • Use data structure library instead of implement from scratch? If implement then put in sdk-utils?, (and test with rapid?)
  • if it's in common utility it should be common, but data structure seems pretty specialized to use case

Other

  • possible to dry out endblock and AfterEpochEnd?
  • don't interpolate data into error strings
  • golang support iterators natively in 1.23 did you consider?
  • general weighted pagination maybe better called weightedPagination or something
  • x/streamer/keeper/abci_test.go is huge, would it have been possible to use rapid?
  • confusing between weight of gauages and weight of iteration

x/streamer/keeper/abci.go Outdated Show resolved Hide resolved
x/streamer/keeper/distribute.go Outdated Show resolved Hide resolved
x/streamer/keeper/distribute.go Outdated Show resolved Hide resolved
x/streamer/keeper/distribute.go Outdated Show resolved Hide resolved
x/streamer/keeper/distribute.go Outdated Show resolved Hide resolved
utils/pagination/paginate.go Outdated Show resolved Hide resolved
x/streamer/keeper/distribute.go Outdated Show resolved Hide resolved
x/streamer/keeper/distribute.go Outdated Show resolved Hide resolved
x/streamer/keeper/distribute.go Show resolved Hide resolved
x/streamer/keeper/distribute.go Outdated Show resolved Hide resolved
@keruch
Copy link
Contributor Author

keruch commented Sep 17, 2024

@danwt thanks for the review!

regarding slices/linked lists. my point about re-allocations was that in case of multiple appends the slice can trigger re-allocations (we don't know the number in advance, but it might be 0-100.000 approx), and it might be inefficient at some scale compared to the linked list. in theory it seems reasonable, but in practice apparently it's not :)
i tried to replace a linked list in my cache with a slice and benchmark it. so you are right, slice seems to be more efficient even with a larger number of elements. i guess due to its more hardware-friendly nature and optimizations.

so i used a slice for this cache after all.

possible to dry out endblock and AfterEpochEnd?

don't think so, they have different semantics though they are very similar. i think i've done the best i could

don't interpolate data into error strings

i got your point, tried to avoid it

golang support iterators natively in 1.23 did you consider?

yes i did, they seem over-engineering for me

general weighted pagination maybe better called weightedPagination or something

kept just pagination, but added a detailed comment + renames weight to operations

x/streamer/keeper/abci_test.go is huge, would it have been possible to use rapid?

it's possible, but i'm afraid that writing them is time consuming. i created a task for it to take on in the future #1231

confusing between weight of gauages and weight of iteration

fixed! see my comments above

x/streamer/keeper/abci.go Dismissed Show dismissed Hide dismissed
x/streamer/keeper/abci.go Dismissed Show dismissed Hide dismissed
@danwt danwt self-requested a review September 18, 2024 14:08
danwt
danwt previously approved these changes Sep 18, 2024
Copy link
Contributor

@danwt danwt left a comment

Choose a reason for hiding this comment

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

good work monsieur

utils/cache/ordered.go Outdated Show resolved Hide resolved
Copy link
Contributor

@danwt danwt left a comment

Choose a reason for hiding this comment

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

lgtm nice work

Copy link
Contributor

@zale144 zale144 left a comment

Choose a reason for hiding this comment

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

beautiful

@danwt danwt merged commit 14877e9 into main Sep 20, 2024
102 of 124 checks passed
@danwt danwt deleted the kirill/354-streamer-distributes-immediately branch September 20, 2024 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants