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

cmd, core, eth: supply delta tracking #25662

Closed
wants to merge 8 commits into from

Conversation

alextes
Copy link

@alextes alextes commented Sep 1, 2022

By all means, ignore this PR for now and focus on the merge.

This PR is based on #24723 by @karalabe / @holiman , however that PR was getting stale, and https://ultrasound.money needs this code to keep track of various models that take the current ETH supply as an input. We've been running the fork in production for a while, we can't expect you busy gethians to prioritise it now. So here we are. I was planning on using a private fork, some things broke in the rebase, after putting in the effort to properly understand what was going on and fix things, I felt I might as well contribute back the code.

This PR adds the following on top of #24723

  1. rewrite a commit to fix a seemingly accidentally removed flag (DocRoot).
  2. rewrite a commit to use new flag fns, some larger shift around updating to urfave/cli/v2 I believe.
  3. fix various typos.
  4. update to new trie.New interface requesting an owner arg (please closely review this change, I'll mark it in the diff).
  5. rename many bits as proposed by @JustinDrake and @holiman.
  6. add a parentHash to the supply delta notification.

I'm not married to anything in this PR. Feel free to edit, request edits, propose to drop some of the renamed bits, all fine 👍 .

I don't normally write golang, but apart from 2. and 3. everything logically appears unchanged to my untrained eyes.

Many blessings for the merge core devs 🐼 🙏 ✨ !

🦇🔊

@holiman
Copy link
Contributor

holiman commented Mar 14, 2023

triage discussion: let's try to get this PR into shape, and get it merged.

@alextes alextes force-pushed the supply-delta branch 4 times, most recently from aa7823a to df0d38f Compare March 15, 2023 13:57
@alextes
Copy link
Author

alextes commented Mar 15, 2023

Rebased against the latest master.

Added three commits to fix code which missed a migration to some new interface. It builds again.

I could work those last three in using rebase if desired, I tried, things got messy, and decided it's not worth my time right now.

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

I have not tested it live, but looks good to me

@MariusVanDerWijden
Copy link
Member

I (hastly) rebased this on master and added withdrawal tracking for the shanghai hardfork here: https://github.com/mariusvanderwijden/go-ethereum/tree/supply-delta @alextes could you rebase this and maybe add the withdrawal tracking changes as well? (please don't cherry-pick my commit, the messages are pretty bad ;))

@alextes
Copy link
Author

alextes commented Apr 19, 2023

@MariusVanDerWijden ya, they looked a bit messy haha (committing merge conflict markers 😅 ?) I did see you add a newline which is 👌 . Code needs room to breathe 😌 .

Anyway, more on-topic, why calculate withdrawals, log them, but then not emit them? (they aren't in the subscribed notification). Do devs benefit from seeing the logged withdrawals? The latter is really the only case I can think of where this is working as intended 😄 . If yes, 👍 . Otherwise, happy to add them to the notification.

@MariusVanDerWijden
Copy link
Member

Yeah that code was written 5 minutes before the shanghai hardfork to make sure everything looks okay wrt. issuance. The primary goal was to be able to see the withdrawal amount in the console. If you think its useful to expose that everywhere, please do!

@alextes
Copy link
Author

alextes commented Apr 20, 2023

Congrats on getting it in, in that case 😅 .

I have no use for withdrawal notifications, we track withdrawals on the beacon side. I'd suggest leaving the additional functionality out then. Features with zero users are 🙅.

@lightclient
Copy link
Member

@alextes just checking on this PR - is this something that is still important for your group? I can look into getting this in shape for merge if so.

@lightclient lightclient self-assigned this Jul 12, 2023
@alextes
Copy link
Author

alextes commented Jul 12, 2023

@lightclient yup, we still run a node based on this branch and to my knowledge anyone that wants to figure out exactly how much ETH is in existence needs this code.

@lightclient
Copy link
Member

Just curious, how do you utilize this? Do you then have an additional diff where you expose this data over RPC? Or is this command all you need to stop maintaining a fork?

@alextes
Copy link
Author

alextes commented Jul 14, 2023

@lightclient it already works as is. You can see an example in the original PR #24723. You add the --supplydelta flag so deltas are stored for every newly synced block. Then this method existing on the api object at all is enough for one to be able to consume a RPC subscription. These docs describe it. The original PR again has an example using netcat. Not the most obvious way of adding an rpc subscription but it is nice and terse 😄 .

@lightclient lightclient force-pushed the supply-delta branch 2 times, most recently from 716c0a1 to cd0e317 Compare July 23, 2023 21:59
karalabe and others added 2 commits July 23, 2023 16:13
Co-authored-by: Péter Szilágyi <peterke@gmail.com>
Co-authored-by: lightlclient <lightclient@protonmail.com>
Co-authored-by: Alexander Tesfamichael <alex.tesfamichael@gmail.com>
@lightclient
Copy link
Member

lightclient commented Jul 28, 2023

This is ready for review again, cc @karalabe @holiman.

I rebased against master and made the following tweaks:

  • added a test in blockchain_tests.go
  • renamed package from core/supplydelta to core/supply since there were more methods in there than just supply
  • updated the function signatures a bit for the provided methods in the package
  • combined the fixed rewards with the uncle rewards (asked Justin, he is okay with this), and it makes more sense this way to me
  • added new geth_* rpc namespace to register the method under -- since this is such a geth-specific method this seems to make sense

Manually verified the output against the ultrasound website for both the logs and subscription and the burn matches what is shown there. @alextes if you have the ability to take a look at this branch and make sure it works correctly from your perspective, it would be appreciated!

@holiman
Copy link
Contributor

holiman commented Aug 1, 2023

Triage discussion: put on hold, we want to see if this whole feature can be replicated using the new tracer-features (live chain tracing) and new event-hooks. Then we wouldn't need to have this as a 'native' geth-command.

#27629

cc @s1na

@alextes
Copy link
Author

alextes commented Mar 13, 2024

Leaving a note I did do another rebase of my own version of this fork for Dencun. Heard from @s1na this may well be the last time I need to 🙌 .

Sidenote: My fork diverged from this one as updated by lightclient. To avoid a DB migration I used a private one.

@s1na s1na mentioned this pull request Mar 26, 2024
@karalabe
Copy link
Member

karalabe commented Jun 3, 2024

Guess this is not needed any more with the supply delta on master now.

@karalabe karalabe closed this Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants