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

transaction generated for stake unlock #1364

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

darcys22
Copy link
Collaborator

Addressing #1351

So I've modified the unlock transaction to include a transaction sent to the users own address.

Attempting to resolve the issue using the second option:

Require that unlock txes have an output, and thus also a fee. Thus an unlock would now have 1-output back into the originating wallet, which would make the wallet see it.

However I am also of the opinion that the 3rd option of adding a burned amount to each stake unlock is a good idea.

Could I get some feedback on if this is the right path to be taking to generate an output for the unlock stake transaction?

src/wallet/wallet2.cpp Outdated Show resolved Hide resolved
Copy link
Member

@jagerman jagerman left a comment

Choose a reason for hiding this comment

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

A couple of additions that would be useful here:

  • a constant unlock burned fee added to cryptonote_config.h that gets added to the TX here, beginning if height indicates HF17+, if that constant is set to some amount > 0. For now we can probably just set it to some small amount (say 0.5 LOKI).
  • enforcement of an output + required burn amount when processing an unlock tx. If the tx has no output, or has a burn amount too low, we should ignore the unlock details in the tx. (Again, this should only kick in starting at the first HF17 block).

@darcys22
Copy link
Collaborator Author

Have updated with the comments, few notes:

  1. Added the UNSTAKE_BURN_FIXED constant to oxen_economy.h and created a new section to store staking fees constants. I saw a note on the get_staking_requirement() function in service_node_rules.cpp that it should be moved to oxen_economy also. It currently has a lot of tests for fork versions which might be due for removal in the next hard fork which would simplify it and make it easier to move here also
  2. I've added the burn amount to the tx_params but unsure if this is the only step (Does an amount also need to be added to the amount field to account for this burn fee?
  3. To check that the burn was included there is already a function in blockchain.cpp Blockchain::check_fee() does this handle the check or will an additional check be needed?

@darcys22 darcys22 force-pushed the 1351-failed-unlock-requests branch from b549cd6 to 3652ee7 Compare March 1, 2021 03:28
@darcys22 darcys22 force-pushed the 1351-failed-unlock-requests branch from b268615 to 7d2159a Compare April 6, 2021 06:08
@jagerman
Copy link
Member

jagerman commented Apr 7, 2021

I think we should hold off on merging this PR until post-HF. It really is something that goes along with oxen-io/oxen-improvement-proposals#27 which is slated for the next PR.

@jagerman jagerman added this to the Oxen10 milestone Apr 7, 2021
@jagerman jagerman removed this from the Oxen10 milestone May 27, 2022
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.

2 participants