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

Check if we can spend or stake remote staking outputs #517

Merged

Conversation

Nizametdinov
Copy link
Member

This PR implements IsMine for remote staking and introduces IsStakeableByMe which checks if an output with given scriptPubKey can be used as a stake.

@Nizametdinov Nizametdinov self-assigned this Jan 31, 2019
@Nizametdinov Nizametdinov requested a review from a team January 31, 2019 14:02
@Nizametdinov
Copy link
Member Author

At first, I introduced ISMINE_STAKEABLE but after merging with ISMINE_HW_DEVICE I ended up with the following enum:

enum isminetype
{
    ISMINE_NO = 0,
    ISMINE_WATCH_ONLY = 1,
    ISMINE_STAKEABLE = 2,
    ISMINE_SPENDABLE = 4,
    ISMINE_FULL = ISMINE_STAKEABLE | ISMINE_SPENDABLE,
    ISMINE_HW_DEVICE = 12, // 0b1100, implies ISMINE_SPENDABLE
    ISMINE_HW_FULL = ISMINE_STAKEABLE | ISMINE_HW_DEVICE, // spendable with hardware wallet and stakeable
    ISMINE_ALL = ISMINE_WATCH_ONLY | ISMINE_FULL
};

I thought that it looked horrible and decided to rewrite it and to introduce a separate function IsStakeableByMe.

@Nizametdinov Nizametdinov requested a review from a team February 1, 2019 11:31
Copy link
Member

@cmihai cmihai left a comment

Choose a reason for hiding this comment

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

utACK ea295d2.

Note on your worries about the enum: ISMINE_HW_FULL = ISMINE_STAKEABLE | ISMINE_HW_DEVICE is not a real-life usecase, because it would require the user to unlock his hardware wallet every time he wants to propose a block, and we want this to be a periodic, automatic process. So the enum would only need one additional member, ISMINE_STAKEABLE.

The solution with IsStakeableByMe is okay too, though.

@Nizametdinov
Copy link
Member Author

@cmihai ISMINE_HW_FULL = ISMINE_STAKEABLE | ISMINE_HW_DEVICE is possible when some remote staking has a non-hardware staking key and a hardware spending key. In such case, the user will need to unlock her hardware wallet for spending but not for staking.

Copy link
Member

@scravy scravy left a comment

Choose a reason for hiding this comment

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

utACK ea295d2

Signed-off-by: Azat Nizametdinov <azat@thirdhash.com>
Wallet CWallet::AddToWalletIfInvolvingMe calls IsMine to check if a
transaction should be added to the wallet. This commit adds
IsStakeableByMe to IsMine so remote staking outputs which are stakeable
but not spendable will also be added to the wallet.

Signed-off-by: Azat Nizametdinov <azat@thirdhash.com>
Signed-off-by: Azat Nizametdinov <azat@thirdhash.com>
Signed-off-by: Azat Nizametdinov <azat@thirdhash.com>
Signed-off-by: Azat Nizametdinov <azat@thirdhash.com>
@Nizametdinov Nizametdinov force-pushed the remote-staking-isstakeable branch from ea295d2 to 22b1e2a Compare February 4, 2019 16:27
@scravy
Copy link
Member

scravy commented Feb 4, 2019

utACK 22b1e2a

Thanks for addressing these!

Copy link
Member

@Gnappuraz Gnappuraz left a comment

Choose a reason for hiding this comment

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

LGTM, utACK 22b1e2a

@Nizametdinov Nizametdinov merged commit af555b8 into dtr-org:master Feb 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants