Proposal : add the possibility to create auction-related transactions in advance

[HDardenne-HNS]

At the moment, if I bid on a name now, I can't create the reveal and redeem transactions before the the name gets respectively in the reveal period and close period. That is, unless I use a web wallet (Namebase for example), I have get back X blocks later to create the reveal (and send it) and get back again Y blocks later for the redeem (and send it). I think we can keep the REGISTER out of the scope.

The goal of this PR is to add the possibility to somewhat automate the auction process. The user will still have to either keep a personal hsd open to send the transactions at the right time or trust someone else to do it (sometimes referred as a kind of watchtower).

If it is accepted, a user could create a bid (and broadcast it immediately or not), and prepare the corresponding reveal and redeem transactions in advance. Then he could either send them at the right time or delegate this action to a third party. It could make the bidding experience on personal wallet less tricky (the principal concern I heard being the reveal that is not made automatically by the blockchain and people being afraid to forget to do it)

[coveralls]

Pull Request Test Coverage Report for Build 532057890

• 59 of 61 (96.72%) changed or added relevant lines in 2 files are covered.
• 5 unchanged lines in 3 files lost coverage.
• Overall coverage increased (+0.09%) to 59.687%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/wallet/http.js	20	21	95.24%
lib/wallet/wallet.js	39	40	97.5%

Files with Coverage Reduction	New Missed Lines	%
lib/protocol/consensus.js	1	82.79%
lib/utils/binary.js	1	56.9%
lib/net/pool.js	3	32.49%

Totals
Change from base Build 518441624:	0.09%
Covered Lines:	19576
Relevant Lines:	30568

---

💛 - Coveralls

[tynes]

This is a good idea, concept ack for sure. I think it would be possible to create invalid transactions if the force param was used incorrectly.

[pinheadmz]

Great work on this!
Also kinda closes #93 or at least should be linked.

A few questions:

1. You modified bid, reveal and redeem in the rpc module but in the http module, only the corresponding endpoint for reveal was modified. I think we should either just pick one set (rpc or http) and the leave the other alone, or do both equally. Sorry this is an annoying aspect of hsd's multi-API structure.

2. In the http endpoint we also have broadcast and sign options, and note how sign is required when broadcasting:

assert(broadcast ? sign : true, 'Must sign when broadcasting.');

...should force also require that sign=true? Or broadcast=false?

3. I think we should not add a force to redeem. There is no time constraint on that covenant so it's not urgent enough to need an external agent (watchtower) to broadcast on a user's behalf. In addition, in your PR you correctly bypass the auction-state check but the wallet will still try to determine if it lost the auction which could be funky if the auction isn't over yet.

4. REDEEMs spend from REVEALs - and if we are presigning a reveal before broadcasting that means the UTXO wont actually be in the wallets database yet, and so it wont be able to construct a REDEEM even if there is a force option, it just wont have the coin to spend. This could also be an issue if a user tries to pre-sign BIDs and REVEALs both before the auction even starts - the wallet will not be able to construct the REVEAL until the BID has been seen by the wallet (successfully broadcast). This is not impossible to do but I think it will require more tools, maybe outside of hsd, to construct a complete chain of pre signed, non broadcasted covenants

So due to 3 & 4 I kinda think we should just limit this PR to presigned REVEALs. The operation won't work unless the corresponding BID has been broadcast and processed by the wallet and if we create a scenario where the BIDs don't exist in the wallet yet we're going to increase complexity and user confusion. REVEAL is the real crucial one anyway (potential fund loss) and the more urgent issue. I can see a version of Bob where every time you place a BID, the wallet also returns the pre-signed reveal tx in hex to the user. User can send the blob via SMS or something to a full node service when bidding ends or any time before.

[HDardenne-HNS]

1. You modified bid, reveal and redeem in the rpc module but in the http module, only the corresponding endpoint for reveal was modified. I think we should either just pick one set (rpc or http) and the leave the other alone, or do both equally. Sorry this is an annoying aspect of hsd's multi-API structure.

The only assert I had in mind was for reveal, hence I just modified the reveal endpoint. If there is additional assert, will add them. If there is something wrong in my reasoning, let me know.
Still, I forgot to change the fromValidator function to include the force option. Will change this.

2. In the http endpoint we also have broadcast and sign options, and note how sign is required when broadcasting:

assert(broadcast ? sign : true, 'Must sign when broadcasting.');

...should force also require that sign=true? Or broadcast=false?

Well we could enforce the use of broadcasting=false to at least be sure it is not broadcasted right away. For sign, requiring it or not is OK with me.

3. I think we should not add a force to redeem. There is no time constraint on that covenant so it's not urgent enough to need an external agent (watchtower) to broadcast on a user's behalf. In addition, in your PR you correctly bypass the auction-state check but the wallet will still try to determine if it lost the auction which could be funky if the auction isn't over yet.

Didn't see this, will dig a little on that and see if I can find a workaround or will abandon the REDEEM.

4. REDEEMs spend from REVEALs - and if we are presigning a reveal before broadcasting that means the UTXO wont actually be in the wallets database yet, and so it wont be able to construct a REDEEM even if there is a force option, it just wont have the coin to spend. This could also be an issue if a user tries to pre-sign BIDs and REVEALs both before the auction even starts - the wallet will not be able to construct the REVEAL until the BID has been seen by the wallet (successfully broadcast). This is not impossible to do but I think it will require more tools, maybe outside of hsd, to construct a complete chain of pre signed, non broadcasted covenants

Will make some tests and see what I can come up with.

So due to 3 & 4 I kinda think we should just limit this PR to presigned REVEALs. The operation won't work unless the corresponding BID has been broadcast and processed by the wallet and if we create a scenario where the BIDs don't exist in the wallet yet we're going to increase complexity and user confusion. REVEAL is the real crucial one anyway (potential fund loss) and the more urgent issue.

The goal I was aiming was to automate the auction as much as possible. If we want to do it covenant by covenant instead of multiple at the same time, I am OK. I agree REVEAL is the more important, I just think if we can add the same possibility to other covenants, we should do it.

I can see a version of Bob where every time you place a BID, the wallet also returns the pre-signed reveal tx in hex to the user. User can send the blob via SMS or something to a full node service when bidding ends or any time before.

Exactly the kind of of Use Case I am going for, except I was thinking of sending the hex to a website. More or less the same.

[pinheadmz]

The only problem with adding another parameter is that the rpc commands expect params to be positional-based (not named, like the JSON 2.0 spec actually requires 😬 ) Which means that if you want to include a force value you MUST also include an account value:

Usage:
hsw-rpc createreveal <name> <account> <force>

If you neglect the account parameter, then the word "true" (or whatever you think you put for force) will be interpreted as the account you want the wallet to create the TX from.

The nice thing about the HTTP API is the arguments are named not just positional (there are no CLI commands for these endpoints yet):

curl http://x:api-key@127.0.0.1:14039/wallet/$id/reveal \
  -X POST \
  --data '{
    "name": "<name>",
    "force": true,
  }'

The rpc createreveal endpoint is not documented yet but if we update it like this we should add it to the docs ASAP. And more importantly, the create____ rpc calls DO NOT SIGN! (#145)

...so we might JUST want to focus on the HTTP endpoint, and then add a CLI command in hs-client

The other thing I'm worried about is the "broadcast anyway" logic in fullnode:

hsd/lib/node/fullnode.js

Lines 339 to 344 in a1409dc

	if (err.type === 'VerifyError' && err.score === 0) {
	this.error(err);
	this.logger.warning('Verification failed for tx: %x.', tx.hash());
	this.logger.warning('Attempting to broadcast anyway...');
	this.broadcast(tx);
	return;

This is something I hope we can get rid of ASAP, it only causes problems. And if we are going to make it even easier for users to generate invalid transactions, it really must be taken out.

And finally, my vote is to just update REVEALs for now. The 52-week name rollout is almost over and so the use case of pre-signing BIDs before an auction starts is going to be over soon anyway. The urgent use-case is preventing funds loss from users who cant open Bob and click a button inside of a 10-day window ;-)

The bigger idea of creating a chain of BID->REVEAL->REGISTER/REDEEM all in advance I think is cool, and I think ideally that can be constructed with more complicated rpc calls based on createrawtransaction (see long discussion in #465 as well) or with an external application.

Note that REGISTER (as well as RENEW) covenants must commit to a block hash from the past 6 months, to prevent excessive pre-signing.

[HDardenne-HNS]

I encountered some difficulties with the creation of the REVEAL transaction based on the BID created before (not able to get the bid blind from txdb). To avoid this, I changed my approach and created a new specific HTTP API endpoint. Down the line, we could use this endpoint to also create the remaining transactions (redeem & register).

The new endpoint will create the BID and the REVEAL based on the created bid. Had to change the way makeReveal and fill work to let me specify everything I needed.

Happy to make any changes that could simplify the code.

PS : not sure what to do about the few KO tests. Checked on master and it seem that they were already not passing

[pinheadmz]

DUDE. Really really awesome work on this. Thank you for having the patience for discussion and review, and for following through with good tests.

Besides a few nits and maybe bike-shedding over function names, my biggest suggestion is to leave makeReveal() alone, and just copy that logic into your new function. You already had to copy/paste it inside an if/else, I think we could have better control if the code path was just brand new rather than having options like force in there.

Also see my note about getting the BID covenant and deriving the blind value from there - that should save a lot of work arounds like exporting extra stuff from the txdb module.

Again, really great work and feel free to disagree with any of my comments or push back or ask another reviewer for second opinions!

[pinheadmz]

Oh one more thing - I think we should have a test in the wallet-http-test module as well - and make sure that the raw hex we get back from the new API endpoint can be sent right back to the node HTTP /broadcast endpoint, and confirmed as valid TXs.

[HDardenne-HNS]

Oh one more thing - I think we should have a test in the wallet-http-test module as well - and make sure that the raw hex we get back from the new API endpoint can be sent right back to the node HTTP /broadcast endpoint, and confirmed as valid TXs.

In fact I do have one that seems similar (not sure about the broadcast though, I use the sendrawtransaction RPC call instead but I think it does not verify it ?). I just didn't pushed it. As far as I can tell, I would need to create a new entry to hs-client to call the new endpoint and as it is another module, well, don't really know what to do ...

[HDardenne-HNS]

I think I addressed most of your remarks except

• Name of API endpoint / function as to me this endpoint will be used to create the whole chain of transactions sometime in the futur
• Enforcing a blind value as this seems more like something app/client may want to set up (or not), not sure we should enforce this ourself
• wallet-http-test as I don't know how to call the new API endpoint, got a prototype of it working but had to change the hs-client dependency locally

[tynes]

wallet-http-test as I don't know how to call the new API endpoint, got a prototype of it working but had to change the hs-client dependency locally

You should be able to .post directly on the client, see: https://github.com/handshake-org/hs-client/blob/d82611cead27d7f9586d878438a23ad2005ff8af/lib/node.js#L86

[pinheadmz]

A rebase on master branch should fix the failed test, which was patched here: #531

[HDardenne-HNS]

A rebase on master branch should fix the failed test, which was patched here: #531

Rebased, still got the same 6 tests failing (Miner & the 2 should have the same DB root)

Made the changes you pointed out in your last review and added a test for the HTTP endpoint (that may be a little bit overkill btw)

[HDardenne-HNS]

Thank you for your time, really appreciate it. Cleaned up & improved the test as requested.

[HDardenne-HNS]

Thanks @tynes for your review, made the changes you highlighted.

Also fixed how I get the NameState. When testing it earlier I had a null exception, looked at makeBid to find a fix.

[pinheadmz]

Great work on this as always, we are getting close!
Also if possible it would be great to squash a few of these commits together so the commit history is a bit cleaner and more direct. I'll leave that up to you, I can help or we can squash when we merge.

[pinheadmz]

I did a test of this endpoint using regtest. The first time I ran the test, I actually didn't set the lockup high enough (honest mistake actually) and ended up with an invalid REVEAL, because it spent the exact same coin as the BID.

SO... I am worried about users doing this without providing sufficient fee in the lockup for the REVEAL. They're not getting a warning even, and they're not going to realize their REVEAL will be invalid. Because this endpoint is designed for "lazy" users, it's unlikely they will even realize they need to create a whole new REVEAL before time runs out.

There is one other complication we could add which is, when creating the REVEAL -- we LOCK the coins spent in the BID. Check out the function lib/wallet/wallet.lockCoin(), that could be used here to help in some cases. We might even want to LOCK the coins spent into the REVEAL as well... although the locked coins list gets reset when a node restarts... so there still will be a possibility that the user sends a TX that invalidates the REVEAL by double spending an input they need to pay the fee.

In any case, when talking about pre-signing transactions, we really need to keep track of which coins are being spent. The REVEAL may not be broadcast for several days, and the user's wallet MUST keep track of any coins spent in that REVEAL and remember not to use them before the REVEAL is confirmed.

The absolute best way to deal with this is to broadcast the BID immediately (wallet won't double spend those inputs) and require that there be sufficient lockup to fund the REVEAL.

Might even want to assert() that the REVEAL only spends one input (the BID itself).

Here's my script for testing: https://gist.github.com/pinheadmz/709d6db66a946a9ae274fc793fb4b8d4
If you set the BID too high or LOCKUP too low, the script will fail because of the double-spend issue

[pinheadmz]

micro-nit: missing period at the end (all the other error statements have it)

[HDardenne-HNS]

Had to re-read it two times to spot it --"

[pinheadmz]

nit: there are actually two bids in this test.

[HDardenne-HNS]

Reworded the whole comment to make things clearer

[pinheadmz]

Awesome dude. Just one more commit and I'll approve:

• fix the two nits above
• describe your new api endpoint in the changelog! See https://github.com/handshake-org/hsd/pull/548/files#diff-06572a96a58dc510037d5efa622f9bec8519bc1beab13c9f251e97e657a9d4ed for an example. Doesn't have to be complete documentation, just mention the new endpoint and 1-2 lines describing.

maybe this:

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 9d52a155d..8e0713ddb 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,14 @@
 # HSD Release Notes & Changelog
 
+## unreleased
+
+### Wallet API changes
+
+- Adds new endpoint `POST /auction` that produces BID and REVEAL transactions for
+a given name, optionally pre-signs them and optionally broadcasts the BID. The
+pre-signed REVEAL can be given to a trusted party to broadcast during the REVEAL
+period without further user interaction.
+
 ## v2.3.0
 
 ### Node changes

[pinheadmz]

ACK c969e8f

Show Signature

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK c969e8f89f29a83d4a45d2dacea18bc7ebc9f627
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmAZy3gACgkQ5+KYS2KJ
yTprnA//XxWZJ0OSCOo6NIZW6x8pjzdyli51Scr6r2+G4o7wIbhqIa4wciepUyQl
u14ls/UmwMRCYd0nm45hEf2YC/rC+U1pqD6dFxSTNqQYfJLVcReCMxQguZFgL+WI
XH7VOUVz6LiczjRqQfKzYgumeGT9ee4hnw0iNkdFUu6HJj03UBNZvqukhvNVZsE8
KodpH7hN/4hBxe/kyhcsZ+BbCc3i3tZLoBWNUQaD6BqQhluYeNHHGqV0ORY8UC16
qLec7n/dJgUwHbGZG7oVP3RXIPWGD98e8P2xCeDLHBSuCZ3fmnkXdlihsXGiGUWo
76tt3qEHSNiH68An9EcFenA7dfoSXVRKwP06loVtA1QpU+Gu1ci4ddzeXF9tfnqd
USi2Hy7YZDw1R2e/wA8zNWPNPaPheO5MK2xxXfO34aZiut8TAgfJHThT4PzUAzXp
CDL+4gHbq+VRg/X8xwjSjJjiEnNbnKkpZ0RHOLmc3tHMeqYLRaj7/VKKp6H4YIRo
/yA/PFIBnXlxB+SHVyCTXwtoMEUHPafoD9o2BwO1LqNlGzSwNHMbOuRw6fzB4UGb
Pw529HH9LKvCJ/YQ1qxkxXx8E0c788Ix0MjaW5N4b+MGo0tpSeRsmUgA7pwG5wn2
6tDqKbBfQEG+PdLhDtYnh0x2XREdhqSOztUA97au0UK5+qyeU+8=
=cLPv
-----END PGP SIGNATURE-----

pinheadmz's public key is on keybase

[tynes]

Good work! Thank you for your contributions