Skip to content

Let proposal_update_operation fail if it approves a proposal but the proposal fails to execute #278

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
mastercyb opened this issue May 9, 2017 · 14 comments
Labels
6 Protocol Impact flag identifying the blockchain logic, consensus, validation, etc. 6 Security Impact flag identifying system/user security 6 UX Impact flag identifying the User Interface (UX) feature hardfork

Comments

@mastercyb
Copy link

screen shot 2017-05-09 at 10 27 27

I suppose that proper behavior is not to confirm approval in this case

@abitmore
Copy link
Member

abitmore commented May 9, 2017

@mastercyb
Copy link
Author

Is not UI issue. My point is that the blockchain should not accept such approval throwing error

@abitmore
Copy link
Member

abitmore commented May 9, 2017

It's a feature.

When updating a proposal (add or remove approvals) without a review period(e.g. committee proposals which will only execute on expiration), if the authority threshold is met after the update, the proposal will be tried to execute immediately. If the execution failed, for example due to insufficient fee, the proposal will be left there waiting for future update, or expiration. It will be tried to execute again after another update if still has enough approvals. It will be tried again on expiration. Related code is here

So in your case, you can transfer enough fund to the account then try to approve the proposal again.

Actually, what we need is a method to notify related parties when a proposal is failed to execute.

@abitmore
Copy link
Member

abitmore commented May 9, 2017

@21xhipster IMHO your suggestion makes sense as well. However, changing of behavior requires a hard fork. Hope others join the discussion.

@pmconrad
Copy link
Contributor

IMO the behaviour of the blockchain is OK, because it doesn't break anything. A complete fix in the suggested form is close to impossible, because the problem situation can be created in many different ways.

@abitmore
Copy link
Member

About implementation, for example we can re-throw the fc::exception when the proposal is failed to execute, rather than catching it and do nothing.

@abitmore abitmore changed the title Transaction stuck after approval due to lack of BTS for paying fees Let proposal_update_operation fail if it approves a proposal but the proposal fails to execute Mar 16, 2018
@abitmore abitmore added this to the Future Consensus-Changing Release milestone Mar 17, 2018
@xiangxn
Copy link

xiangxn commented Apr 11, 2018

Is there a good way to log errors and return them to the UI? All I can think of is adding fields to proposals.

@abitmore
Copy link
Member

abitmore commented Aug 19, 2018

Note:

@abitmore
Copy link
Member

Let's get this done in next consensus-upgrade release. Need a BSIP?

@abitmore abitmore added 6 Protocol Impact flag identifying the blockchain logic, consensus, validation, etc. 6 Security Impact flag identifying system/user security 6 UX Impact flag identifying the User Interface (UX) labels Sep 24, 2018
@pmconrad
Copy link
Contributor

At BitFest I was talking with @oxarbitrage about how to automate certain things at the blockchain level, and came up with a use-case that would be destroyed by the proposed change. I think we should keep the current behaviour.

Use-case: Alice wants to automatically transfer a CAKE token to Bob at a specific point in the future (for example as a birthday present).

  1. Alice creates "cron" account
  2. Alice sends 1 CAKE to cron
  3. Alice makes sure cron doesn't have sufficient BTS for a transfer fee
  4. Alice proposes that cron sends 1 CAKE to Bob, proposal expires on Bob's birthday
  5. Alice sends sufficient BTS for proposal_update to cron
  6. cron approves the proposal
  7. Proposal fails to execute immediately due to lack of funds for transfer fee
  8. Alice sends transfer fee to cron
  9. On Bob's birthday the proposal expires and is executed again. Since there is sufficient BTS in cron account for transfer, the execution succeeds and Bob receives his CAKE.

@abitmore
Copy link
Member

@pmconrad I don't think it's a good use case, it's made too complicated. If you want a proposal to be executed only on expiration, you can simply set a review period.

@pmconrad
Copy link
Contributor

That's not my point. I think the proposal would cripple current functionality, and I don't see the need.

It's not a bug, so a BSIP is required in any case.

@abitmore
Copy link
Member

@pmconrad the reason I think we need this is IMHO it can fix the multiple proposal_update issue.

@jmjatlanta
Copy link
Contributor

jmjatlanta commented Dec 7, 2018

Excuse me for being naive, but why not just reply to the caller that the update succeeded, but the proposal has yet to execute successfully? Perhaps with a reason would be even better. I do not see it as an exception, as it is common. I see it as a status.

I believe we have the mechanism to return more than just a simple yes/no result. Does that not give everyone what they want? (no change in functionality, just alert the user)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6 Protocol Impact flag identifying the blockchain logic, consensus, validation, etc. 6 Security Impact flag identifying system/user security 6 UX Impact flag identifying the User Interface (UX) feature hardfork
Projects
None yet
Development

No branches or pull requests

7 participants