Ithaca #581
Ithaca #581
Conversation
…y, attempt to fix test
|
I have not made much progress in the past 48 hours. Current blocker is that I have not found the right formula to calculate the endorsement rewards for a future cycle (only needed in I think we should not merge until running TRD with the same parameters with all 3 providers gives us the same calculations file bit-by-bit. Also, we used to have tests for it, it would be useful to have fresh tests as well, but they can probably come in in another PR. If you want to help, you are welcome to test RPC and tzstats providers (you have to comment out these lines) and report any discrepancies you can find. It's good to test pathological cases such as: double baking (not sure if it has happened yet), lost endorsement rewards, revelation rewards. Try ideal and actual rewards with As of the most recent commit I remain highly confident in this branch with tzkt provider, actual rewards, and |
|
We could always make -R -11 disabled for now, and address it later. |
Manual testing shows that tzkt and rpc give bit-by-bit identical simulations files in `-R -5` and `-R -11`. I am re-enabling rpc.
|
I have re-enabled RPC. I have verified that RPC and TZKT methods give me the exact same simulation csv file, in both There are still a few discrepancies with tzstats being worked on. But, we can probably clean up the tests and merge this soon. There is special-case code in RPC for cycles 468-474 because the stake snapshot for 468-473 is hardcoded to the first ithaca block, and for 474 we use the last snapshot of the cycle 468 (RPC returns wrong info). We have a bunch of pre-ithaca functional tests against mainnet that we need to throw away and redo. But I would rather wait to be out of this initial ithaca phase (cycles 468-474) before hunting for interesting mainnet test cases. So, I think we can test a little bit, fix the failing tests, then merge this. |
* Use the snapshot cycle first block level to get the block. With short cycles, the current level might be in a different cycle, causing the RPC to fail. * fix black Co-authored-by: Nicolas Ochem <[email protected]>
* Contributor: novalis, Effort=1h
* Ithaca constants added for mainnet and testnet * Updated URLs to get values from the nodes
|
@nicolasochem did you test with tzstats? |
| if release_override not in [ | ||
| estimated_reward_override, | ||
| frozen_reward_override, | ||
| 0, |
There was a problem hiding this comment.
Is 0 now deprecated? Shouldn't we exclude it here and make -5 as default?
There was a problem hiding this comment.
yes, 0 is now deprecated, and I agree -5 should be the default, but there are complications. I thought we could take the opportunity to migrate this parameter to the config file. We talked about this before, and you said we should have a generic way to pass parameter with either the config file or the CLI. I don't disagree, I think it requires big refactoring which is better done after this PR is merged. Let's leave the default to 0 now.
A little bit but I found discrepancies. It is currently disabled. I suggest we merge and fix it later. |
Support ithaca and the current tenderbake consensus mechanism.
Consensus changes
Ithaca brings extensive changes to the reward calculation which has an impact on TRD:
This caused extensive changes in the payment_producer code:
This branch is already being used by most TRD users since master does not work on ithaca - 3 cycles have been paid so far, no one reported any horribly wrong miscalculation. Still, proper review is required.
Note: tzstats provider is disabled for now, because some of the values returned by the api do not match tzkt/rpc. This is under investigation, but let's not hold for this. We will fix tzstats later.
Note: ithaca makes it possible again to pay rewards with RPC. During the emmy+ to emmy* transition, this became all but impossible because the individual endorsing rights (256 per block) were so numerous than the query was making the node hang. All public tezos rpc providers ended up blacklisting this rpc endpoint, so everyone switched to tzkt/tzstats. No longer! You still need an archive node though, and it's slow, but I think it's important to preserve this capability.
Beginning of ithaca - special case
Cycles 368-373 are special because they are all using the first ithaca block as the snapshot block. Also, cycle 374 is special because the snapshot information returned by the node is wrong. This has been hardcoded in the RPC code (indexers handle this internally). We will remove this code later.
Fee changes
The fees changed slightly in ithaca, the fee calculations were adjusted after a few bakers reported payout failures. @jdsika may fill in the details.
Manual tests
I have verified with several baker addresses that tzkt and rpc give me exactly the same calculation csv file, in
idealandactualrewards, in-R -5and-R -11(early payout) model. The calculations look good, and I have done several real payments with the branch.Reviewers should perform more of these tests.
Ithacanet
Dev on this branch was originally made on ithacanet, so the code was changed to support ithacanet instead of hangzhounet. I have not tried again since switching to master. Some tests were dependent on hangzhounet, I have deleted them as the model changed.
Automated tests
I axed most of the regression tests with the intention of rewriting them later. These tests were making calculations on various Emmy cycles on mainnet, using mockups of RPC queries. These cycles are no longer relevant in ithaca, so these calculations were broken. Plus, the rpc, tzkt, tzstats tests were written organically and there is no consistency between them. I have written a plan to overhaul the payment_producer integration tests in #547.
We should merge before these tests are rewritten because everyone is already using the branch anyway; and I would like to wait until we are outside of the 6 initial cycles of ithaca before capturing interesting mainnet examples that we could turn into tests.
Docs
Remove mentions of "frozen" and "freezing", also update the example of adjusted early payouts to match the newer tenderbake calculation.
General TODOs and changelog:
Note: some improvements are cherry-picked on master
Contributor:
nicolasochem = 30h
jdsika = 8h
Awards (please get in touch with me on slack if a meaningful helper is missing):
@sebuhbitcoin 1h
@NeoktaLabs 1h
@pea-io 1h
@novalis 4h