-
Notifications
You must be signed in to change notification settings - Fork 717
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
[wallet] Extra GetTransaction call in stakeable coins selection flow removed. #1850
[wallet] Extra GetTransaction call in stakeable coins selection flow removed. #1850
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a rebase. Otherwise looking pretty good. Just a few points.
920f17c
to
f61e71f
Compare
Rebased on top of master. |
f61e71f
to
5260c12
Compare
…e entire transaction.
…d cleaning the output availability checks from AvailableCoins
…nd returning CStakeableOutput.
Before was loaded in GetIndexFrom and such functionality was removed.
5260c12
to
06bae43
Compare
rebased on master. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Much better with CPivStake immutable.
Just few notes.
06bae43
to
5935497
Compare
updated per feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per feedback inline, I think InitFromTxIn
shouldn't be used for piv stakes (it isn't intended to "check if the object is initialized" anyway, it was meant for setting things)... you could just override it to false in the header.
Similarly GetTxOutFrom
could be easily changed in the base class, and overriden to return an empty object, for zpiv stakes, where it's not used.
But these are just styling changes, so if you're not happy making them, I'm fine with the PR as is. Tested ACK 5935497
Hey, was answering and didn't see this last comment. I'm good with almost every comment, i thought on some of them as well but.. this PR is already fulfilling its main goal of removing the GetTransaction function call. Better to leave them for another refactoring round. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 5935497
Essentially, this work is removing an unneeded
GetTransaction
call fromCPivStake::GetIndexFrom
for every stakeable output during each cycle of the staking process.As
GetTransaction
is a direct access to disk (txindex), this means faster staking.Future work, the methods
StakeableCoins
andAvailableCoins
can be abstracted even more. They are sharing almost the same code with an specific distinction that makes this change possible without introducing an extra blockIndex set intoAvailableCoins
.