Skip to content

CIP-0058 (Script - 2) Add weight as input + test for invalid weight #1587

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jose-velasco-ieu
Copy link
Contributor

@jose-velasco-ieu jose-velasco-ieu commented Jul 18, 2025

🧾 Summary

This PR improves the unclaimed_sv_rewards.py script to support milestone-based reward accounting by allowing fine-grained weight input and enforcing correct validation.

The script summarizes claimed, expired, and unclaimed minting rewards for a given beneficiary within a specified time range and weight.

This pull request is an extension of #1356


✅ Key Improvements

  • Adds and Validates --weight input: Ensures the provided weight is not greater than any eligible reward coupon’s weight.
  • Fix: Removed duplicated error logging (errors were previously logged in both _fail and via sys.excepthook).
  • Add test use case for running the script with invalid weight.

📌 Example: Ghost-party-A Milestones

⏱ Timeline

   t0                    t1                 t2                      t3
   |   m₁ (w=1)          |   m₂ (w=1)       |   m₃ (w=2)            |
---------------------------------------------------------------------------------
   |444444444444444444444|333333333333333333|22222222222222222222222|00000...

🧩 Steps

  • At t0:

    • GSF sets Ghost-party-A weight = 4
  • At t1 (m₁ is completed):

    • GSF sets weight = 3
    • GSF runs the script with:
      --begin-record-time=t0 --end-record-time=t1 --weight=1
      
    • A vote is initiated for m₁
  • At t2 (m₂ is completed):

    • GSF sets weight = 2
    • GSF runs the script with:
      --begin-record-time=t0 --end-record-time=t2 --weight=1
      
    • A vote is initiated for m₂
  • At t3 (m₃ is completed):

    • GSF sets weight = 0 (party removed)
    • GSF runs the script with:
      --begin-record-time=t0 --end-record-time=t3 --weight=2
      
    • A vote is initiated for m₃

This enables rewards to be split according to milestone weights and transitions over time.

Pull Request Checklist

Cluster Testing

  • If a cluster test is required, comment /cluster_test on this PR to request it, and ping someone with access to the DA-internal system to approve it.
  • If a hard-migration test is required (from the latest release), comment /hdm_test on this PR to request it, and ping someone with access to the DA-internal system to approve it.

PR Guidelines

  • Include any change that might be observable by our partners or affect their deployment in the release notes.
  • Specify fixed issues with Fixes #n, and mention issues worked on using #n
  • Include a screenshot for frontend-related PRs - see README or use your favorite screenshot tool

Merge Guidelines

  • Make the git commit message look sensible when squash-merging on GitHub (most likely: just copy your PR description).

Comment on lines +619 to +620
self._verify_weight(reward)
amount = self.weight * mining_round_info.issuance_per_sv_reward
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems dangerous to me. Consider this:

Rewards get issued for weight 10.

You then run the script with weight 7 (let's assume this was a milestone or something). This succeeeds.

You then rerun it with script weight 6. This will also succeed.

But clearly the totals don't match up.

Now you can argue that this will never happen in practice but it doesn't seem that unlikely to me. Just a misconfiguration in the beneficiaries could result in it. Or you've changed the weight sometime in the time range deliberately but you started running the script before that when the weight was not effective yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point. However, if the user runs the script with invalid parameters, the other SVs can reject the vote based on the number provided by the user requesting the vote. I don’t see a way to fully protect against erroneous or malicious inputs.
cc: @waynecollier-da

Copy link
Contributor

Choose a reason for hiding this comment

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

I see your point. However, if the user runs the script with invalid parameters, the other SVs can reject the vote based on the number provided by the user requesting the vote

Sure but I have no idea how I as an SV can detect that the parameters are invalid so how should I know to reject it?

I think the problem with the weight option you implemented is that it assumes you can run independent queries with different weights whereas the safe option is to figure out how much you want to mint for that and then only look at the remainder for your next query. But with the script I don't know what the remainder was so I have to rerun it normally which might lead to me double counting some portion of rewards.

The only case where your approach is safe is if the weight stayed constant all the time. So maybe one option is that you add a flag for the expected total weight and assert on that each time you see one created. That guards against mistakes.

It seems like the cleanest option would be to actually use different parties for different milestones but I guess we didn't do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like the cleanest option would be to actually use different parties for different milestones but I guess we didn't do that?

Unfortunately, a single "ghost-xxx" party is used for all the milestones. The way GSF reduces the weight after each completed milestone is as follows:

   t0                    t1                 t2                      t3
   |   m₁ (w=1)          |   m₂ (w=1)       |   m₃ (w=2)            |
---------------------------------------------------------------------------------
   |444444444444444444444|333333333333333333|22222222222222222222222|00000...

So the weight does not stay constant all the time.

cc: @waynecollier-da

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok so how about this:

My concern as I explained above is that you might double count stuff twice and I don't see a good way for me as an operator to prevent this with just the weight option. What if we add an already-minted-weight option that you subtract first and then only the remainder can be consider for the current weight option? As an operator I can reasonably be expected to set already-minted-weight option and if it is set, it is always subtracted first so I'm not gonna double count anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@moritzkiefer-da
I'm not sure if I'm following you.

subtract first

Do you mean subtract from the processing coupon weight?
Correct me if I'm wrong. The check would like that:

coupon_weight - already_minted_weight (input) >= weight (input)

   t0                    t1                 t2                      t3
   |   m₁ (w=1)          |   m₂ (w=1)       |   m₃ (w=2)            |
---------------------------------------------------------------------------------
   |444444444444444444444|333333333333333333|22222222222222222222222|00000...

For example in this scenario:

At t1:
-begin-record-time=t0 --end-record-time=t1 --weight=1 --already-minted-weight=0
Between t0 and t1: 4 - 0 >= 1 ---> OK

At t2:
-begin-record-time=t0 --end-record-time=t2 --weight=1 --already-minted-weight=1
Between t0 and t1: 4 - 1 >= 1 ---> OK
Between t1 and t2: 3 - 1 >= 1 ---> OK

However at t3:
-begin-record-time=t0 --end-record-time=t3 --weight=2 --already-minted-weight=2
Between t0 and t1: 4 - 2 >= 2 ---> OK
Between t1 and t2: 3 - 2 >= 2 ---> FAIL
Between t1 and t2: 2 - 2 >= 2 ---> FAIL

I think we are still missing something, because the already-minted-weight depends on the time period.
Or maybe I didn't get your idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we are still missing something, because the already-minted-weight depends on the time period.

Can't I just run it with different already minted weights across different time periods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.
Do you agree with this?
@moritzkiefer-da @waynecollier-da

   t0                    t1                 t2                      t3
   |   m₁ (w=1)          |   m₂ (w=1)       |   m₃ (w=2)            |
---------------------------------------------------------------------------------
   |444444444444444444444|333333333333333333|22222222222222222222222|00000...

CHECK: In each time period, coupon_weight - already_minted_weight (input) >= weight (input)

Milestone 1 (m₁) calculation at t1 (single execution):

  • t0 to t1
--begin-record-time=t0 --end-record-time=t1 --weight=1 --already-minted-weight=0  # 4 - 0 >= 1  (OK)

Milestone 2 (m₂) calculation at t2 (2 executions):

  • t0 to t1
--begin-record-time=t0 --end-record-time=t1 --weight=1 --already-minted-weight=1  # 4 - 1 >= 1  (OK)
  • t1 to t2
--begin-record-time=t1 --end-record-time=t2 --weight=1 --already-minted-weight=0  # 3 - 0 >= 1  (OK)

Milestone 3 (m₃) calculation at t3 (3 executions):

  • t0 to t1
--begin-record-time=t0 --end-record-time=t1 --weight=2 --already-minted-weight=2  # 4 - 2 >= 2  (OK)
  • t1 to t2
--begin-record-time=t1 --end-record-time=t2 --weight=2 --already-minted-weight=0  # 3 - 1 >= 2  (OK)
  • t2 to t3
--begin-record-time=t2 --end-record-time=t3 --weight=2 --already-minted-weight=0  # 2 - 2 >= 2  (OK)

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