wallet: accept account argument for all covenant makers and verify account ownership during coin selection #233
Conversation
Codecov Report
@@ Coverage Diff @@
## master #233 +/- ##
==========================================
+ Coverage 60.35% 60.96% +0.61%
==========================================
Files 129 129
Lines 34677 34728 +51
Branches 5862 5894 +32
==========================================
+ Hits 20929 21172 +243
+ Misses 13748 13556 -192
Continue to review full report at Codecov.
|
pinheadmz
changed the title
|
pinheadmz
force-pushed
the
reveal1
branch
3 times, most recently
from
b0b02ae
to
ec2bdc8
Compare
|
Updated to cover sending |
|
@wi-ski Do these changes fix any issues you are seeing? |
|
@tynes - Yes sir they do. |
|
Awesome job @pinheadmz 👏 |
pinheadmz
changed the title
|
@tynes @pinheadmz - Dusting off this set of changes :) Is there any chance these are still ok? Happy to take them over the line. |
|
@chjj @boymanjor @pinheadmz @tynes - Can we please get eyes on these changes? |
|
rebase to 5c09c70 |
|
@pinheadmz - Hey! Ty so much for making these changes <3 |
|
I added account support for reveal and redeem, as well as update. I started having second thoughts about how to approach this for owned names: update, register, transfer, finalize, revoke, since once a name is owned, there is only one account that can send the covenant anyway. I was thinking maybe those calls should just not accept an account argument and when it comes to filling the mtx, those calls should just force a But now I think we should still do this (and I'll have to add 3-4 more commits to this PR) and here's why: a user may want the account that owns a name to ALSO pay the mining fee. This and related coin-selection / privacy between accounts reasons. SO for update, etc. we will still accept an account parameter but only TWO options are available: the account that owns the name, or null (fee will be paid by any coin in the wallet, and change always goes to default account). If the wrong account is given, that'll be an error. |
|
In the future we should consider adding an account index for bids. |
|
@chjj yeah some users with lots of activity are reporting long processing time generate REVEALs |
pinheadmz
changed the title
Adds new method hasCoinByAccount(acct, hash, index) to txdb which returns true if the coin (hash, index) is owned by account (number). Adds this check to wallet.makeReveal() to prevent BIDs from other wallet accounts being included as inputs to a new REVEAL TX from a specific account.
also: make renew and make redeem
|
@chjj @boymanjor @wi-ski |
|
@boymanjor we have a non-deterministic test in |
|
@pinheadmz - Wowza. You're a beast. Thank you for hacking at this. @chjj's comment inspired me to investigate intro-ing account indices to bid layouts - this: https://github.com/pinheadmz/hsd/pull/2/files ☝️ Is this in the right 'spirit' for adding an account index for bids? Meaning - appending a new int here? If so, can keep pulling that thread. Im new to LevelDB and the greater-than/less-than mechanics for iterators are not entirely clear to me yet. |
|
That PR is targeting these changes * |
|
@wi-ski I think the idea behind indexing bids is not just adding an integer to the bids in layout, but adding a new index entirely like |
|
Ok - My wishful thinking was wishful. 🙃 |
|
@wi-ski I encourage you to continue looking into this problem! The goal would be: when the wallet goes to make a reveal, instead of iterating through every bid in the wallet, it just looks for the bids in the specified account… How could this be accomplished? 🤔 |
|
Yes sir, Ill continue poking 🤔 |
|
Ty for the guidance |
Closes #231
Adds new method hasCoinByAccount(acct, hash, index) to txdb which
returns true if the coin (hash, index) is owned by account (number).
Adds this check to wallet.makeReveal() to prevent BIDs from other
wallet accounts being included as inputs to a new REVEAL TX from a
specific account.
TODO:
double-reveal-test.jsin a more appropriate place... (auction-test?wallet-test?)UPDATE,REGISTER. etc...accountparameters