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

Remove sequential increment requirements for Child Bounties #4947

Open
2 tasks done
itsbirdo opened this issue Jul 4, 2024 · 6 comments
Open
2 tasks done

Remove sequential increment requirements for Child Bounties #4947

itsbirdo opened this issue Jul 4, 2024 · 6 comments
Labels
I5-enhancement An additional feature request.

Comments

@itsbirdo
Copy link
Member

itsbirdo commented Jul 4, 2024

Is there an existing issue?

  • I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

  • This is not a support question.

Motivation

When you create a child bounty it needs to have a unique ID, which has to be +1 from the last unique ID in the system.
This is fine if there is only one bounty active at a time, but there are many active bounties and we'll see more and more active bounties coming along. This leads to collisions in transactions, resulting in frustration, delays with sending funds to individuals and lost transaction fees.

Example: I create a child bounty, with the id 135, but it takes the signers on the multisig 24h to review and sign the tx.
If another bounty proposes a child bounty with the ID 135 and it gets signed before mine, my transaction will fail. Now imagine this multiplied by many bounties.

As an individual managing a bounty I have had this problem first hand. I know many other bounty managers also struggle with this design decision from how bounties operate.

Request

I would propose to change the way child bounties operate to not use an incrementing ID across all bounties.
Also ensure that child bounties don't need to be sequentially signed based on when they were raised.

Solution

I would propose to move to a user generated GUID, which would reduce collisions to almost zero, however any alternative approach could work.
Another option could be that child bounties need to start with the main bounty number,
e.g. Bounty 37, child bounty 1 (37-1) which is increased sequentially, this results in zero collisions, proving the bounty manager doesn't make a mistake.

Are you willing to help with this request?

Yes!

@itsbirdo itsbirdo added the I5-enhancement An additional feature request. label Jul 4, 2024
@itsbirdo itsbirdo changed the title Introduce GUID for Child Bounties Remove sequential increment requirements for Child Bounties Jul 4, 2024
@ggwpez
Copy link
Member

ggwpez commented Jul 4, 2024

Sorry am not really familiar with the Bounty pallets. Can you post a link maybe to a failed Subscan extrinsic?
Is it the ChildBounties::add_child_bounty call that fails or a different one?

@ggwpez
Copy link
Member

ggwpez commented Jul 5, 2024

Probably its this https://polkadot.statescan.io/#/extrinsics/20813055-2:
Screenshot 2024-07-05 at 15 31 29

They are trying to send two calls in a batch and just guessing the childBountyId. So sure it can be wrong if another addChildBounty call came first. Could be solved by attaching a curator proposal directly to the add_child_bounty call.

Another way would be to add a custom XCM instruction (probably controversial), since you basically want to get the "return value" of the first call as argument for the second one.

@itsbirdo
Copy link
Member Author

itsbirdo commented Jul 9, 2024

It doesn't matter if you try to send two calls in one batch. Please review the use case, this will still happen if you are proposing just one transaction in a child bounty, batch or no batch.

Example: I create a child bounty, with the id 135, but it takes the signers on the multisig 24h to review and sign the tx.
If another bounty proposes a child bounty with the ID 135 and it gets signed before mine, my transaction will fail.

This is why the number needs to be a guid (or something else) and not a sequential increasing number

@ggwpez
Copy link
Member

ggwpez commented Jul 17, 2024

Apparently this kind of TX fails https://polkadot.subscan.io/multisig_extrinsic/21573048-3?call_hash=0xe9a4e2270847ff97d6be19a4ce66dc3bd6985b7f09381dc28a875d1897e8fd04
Quote form Birdo:

this one failed because there was another bounty which executed before this one was signed
using the same number

@paradox-tt
Copy link
Contributor

Hey Oliver,

Perhaps I can lend another, maybe more informed perspective on the discussion.

To award a child bounty requires several steps, they are as follows:

  1. addChildBounty(ParentBountyID, Value, Description)
  2. proposeCurator(ParentBountyID, ChildBountyID, Curator, Fee)
  3. acceptCurator(ParentBountyID, ChildBountyID)
  4. awardChildBounty(ParentBountyID, ChildBountyID, Beneficiary)

If followed sequentially it requires at least two signing activities; the first to add a child bounty (step 1) and the second can be a batch which uses the child bounty id obtained in step 1, to fill parameters of steps 2, 3 and 4. If followed in this manner there is no issue.

Another aspect of consideration is that the curators of most bounties are multi-sig wallets and the final signature on any given step or batch may be obtained serveral hours after a call is initiated. With this in mind as well as the paragraph above, awarding a child bounty can take days.

In practice some curators attempt to batch steps 1 to 4 together and assume that the next child bounty id would be available through out the signing process. With several bounties opening child bounties, there is a highly probability that the child bounty id might be used by another bounty resulting in the batch failing.

Birdo is suggesting that a GUID be assigned to child bounties such that the child bounty ID can be predicted and be applied to steps 2, 3 and 4 without worry of collision.

Imo, I think his suggestion would solve the problem but not in the most efficient manner.
The issue as I see it is not with the use of shared child bounty ids but more so with attempts to batch steps 1 - 4 together. This can be easily overcome by creating a new extrinsic which upon execution obtains the next available child bounty index and applies the actions of steps 2, 3 and 4. This keeps the storage index "small", it doesn't require any further compute on the chain to generate/compare GUID/hashes and of course we don't have to worry about any "migration" type concerns.

There was some discussion on this in 2023 here. I'd love to take a stab at this as part of my portfolio to enter the fellowship. I think I can code this fairly easily but I'm not familiar with the testing QA/QC aspect.

@ggwpez
Copy link
Member

ggwpez commented Jul 22, 2024

The issue as I see it is not with the use of shared child bounty ids but more so with attempts to batch steps 1 - 4 together.

If this solves your pain then please go ahead 🙌

I'd love to take a stab at this as part of my portfolio to enter the fellowship. I think I can code this fairly easily but I'm not familiar with the testing QA/QC aspect.

Yea just open a merge request and ask for reviews in the public fellowship channel. Tests are always a good idea though 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I5-enhancement An additional feature request.
Projects
None yet
Development

No branches or pull requests

3 participants