fidelity bonds foundation: define bond tx structure, create and parse #1818

chappjc · 2022-08-24T20:00:03Z

Further breaking up #1480 for feasible review, this piece deals with:

Definition of the fidelity bond transaction
Creation of the bond and refund txn (for client)
Parsing of the bond txn (for server)

To make this transition smoother, the client and server retain all the legacy registration fee machinery, and the client's continues to use the legacy registration system.

Background from #1480:

This creates a time-locked fidelity bond, which is redeemable by the user who posted the bond after a certain time. This creates a opportunity cost to use DCRDEX instead of a simple monetary cost. It also is a prerequisite for mesh.

The DEX considers a user's bond as active when the bond script's lockTime (when it can be redeemed by the user) is at least bondExpiry in the future. That is, the bond remains locked for a period even after it becomes inactive for DEX purposes.

Bond Transaction Structure (version 0)

There must be at least two outputs: the bond output (P2SH) and an account commitment (nulldata).

See:

dex/networks/dcr.ExtractBondDetailsV0
client/asset/dcr.(*ExchangeWallet).{MakeBondTx,RefundBond}
server/assset/dcr.ParseBondTx

Time-locked Bond (output 0)

Output 0 of the fidelity bond transaction must be a P2SH output paying to the bond script.

The redeem script looks similar to the refund path of an atomic swap script:

<locktime[4]> OP_CHECKLOCKTIMEVERIFY OP_DROP <pubkey[33]> OP_CHECKSIG

The above uses P2PK since nothing is gained from P2PKH on account of the need to reveal the script pubkey anyway for validation on the server. (The user must demonstrate ownership of the locked funds by signing a message with the private key corresponding to the pubkey in the TL redeemscript.)

Server validates that this P2SH output actually pays to the provided redeem script.

The pubkey referenced by the script need not be controlled by the user's wallet, but in this implementation it comes from their wallet. The client could even use their account pubkey in the script, but for security reasons it is best not to reuse pubkeys in this bond outputs, which are unspent on the blockchain for long periods. We may consider using a key pair from the client's HD keychain, but I'm leaning toward this being a wallet job.

The lockTime must be after a bond expiry time, the lockTimeThreshold. A bond becomes expired for a DEX account when duration until lockTime is less than a bondExpiry duration, which is on the order of a month. This, a user should create a bond with a lockTime at least bondExpiry in the future, but a practical lockTime would be more like 2x bondExpiry in the future so the account is active on the DEX for bondExpiry.

DEX Account commitment (output 1)

The account commitment OP_RETURN output should reference the account ID that corresponds to the account pubkey in the postbond request.

Having it in the raw like this allows the txn alone to identify the account, and without requiring the bond output pay to the account's private keys.

Open Questions

Is this transaction structure and account commitment output sufficient to prevent txns from being dual purposed for other services with similar TL output structure?

In the future, how can we modify the txns or outputs to "re-commit" them in new bonds sooner than their locktime expiry, which is well past their dex expiration duration?

client/asset/dcr/dcr.go

JoeGruffins · 2022-09-05T09:07:49Z

Is this transaction structure and account commitment output sufficient to prevent txns from being dual purposed for other services with similar TL output structure?

Would this be a problem? Do you mean using the same bond for multiple.. meshes? Could require the account ID be hashed with some other data. Maybe something specific to that dex.

In the future, how can we modify the txns or outputs to "re-commit" them in new bonds sooner than their locktime expiry, which is well past their dex expiration duration?

The duration logic is not in this pr is it?

Because you can't use the script to decide where the output is spent to, I can't think of an answer at the moment. If conditions are met you can spend to whatever you want, so how do you make a re-lock path.

Maybe bad idea, but something: Make another path that opens earlier that can be spent by a 2:2 multisig script and client sends their side signed to the server and if it looks like they are putting it in a new bond server also signs allowing the bond to be spent to another bond sooner if both parties agree.

dex/networks/dcr/script.go

client/asset/dcr/dcr.go

server/asset/dcr/dcr.go

client/asset/dcr/dcr.go

buck54321 · 2022-09-07T13:13:54Z

client/asset/dcr/dcr.go

+// RefundBond refunds a bond output to a new wallet address given the redeem
+// script and private key. In general, the bond should have been created with
+// this wallet, but this method makes an effort to find and spend any bond.
+func (dcr *ExchangeWallet) RefundBond(ctx context.Context, ver uint16, coinID, script []byte, privKey []byte) ([]byte, error) {


Thinking of UX, I think automatic bond renewal should be a default. Core can handle that in a two step process too, but a one step process, i.e. a RenewBond method, would have some advantages too. Just something to consider.

I agree automatic bond maintenance should be the preferred approach with Core. I allude to this a bit in the integration PR that follows, but haven't done this yet. I'm imagining the settings page will let you set either a target tier or a target bond strength, perhaps in units of the bond increment, plus a choice of the asset to use.

I also think it'll be important from from a UX perspective to allow Core to lock bond funds ahead of time. For the first bond, they'll want to lock up the second, so that there are no surprises. This would necessitate a (Bonder).LockBondFunds method or similar, and of course special handling in core.

buck54321 · 2022-09-07T13:22:54Z

client/asset/interface.go

+	Version               uint16
+	AssetID               uint32
+	CoinID                []byte
+	SignedTx, UnsignedTx  []byte


I would normally expect the Bond to have sufficient data to reconstruct the transaction, and vice-versa. If we're using a wallet output, the tx will always be available. If we're using an account-derived privkey, we'll be passing in the key anyway. What is the advantage to passing around these transactions?

Depending on the answer there, I guess the next question is why have a generalized SendTransaction method on every Wallet instead of a SendBond(*Bond) method on the Bonder interface?

The unsigned tx goes to the server for pre-validation prior to broadcast. It's probably clearer in the comms PR (#1819) and the draft integ PR (#1820).

The struct is the output of the method that constructs the transaction, so there's no real need to reconstruct. This is just all you need to 'postbond'.

chappjc · 2022-09-08T04:08:21Z

Thanks for the great ideas @JoeGruffins.
Will soon be iterating on this one a bit more with the (now very stale) stale integ branch, so more tweaks likely ahead, but it's good to have all the bond details in the clear now.

buck54321

I think this structure seems functional and versatile. Just some comments right now, but I don't see any big change requests going forward.

client/asset/dcr/dcr.go

buck54321 · 2022-09-22T23:42:00Z

client/asset/dcr/dcr.go

+	if lockTimeSec > math.MaxInt32 { // https://en.wikipedia.org/wiki/Year_2038_problem
+		return nil, fmt.Errorf("invalid lock time %v", lockTime)
+	}


I wanted to verify that we're limited to signed 32 bit integers. And I think I did so in both Decred and Bitcoin. But searching online, I found this popular answer to the 2038 question.

https://bitcoin.stackexchange.com/questions/79182/january-19th-2038-rip-unix-timestamps

which I guess is just wrong.

I guess that stackexchange Q&A is referring to the block height integer in the serialized block header, whereas in Script, the variable-length integer encoding has a sign bit.

However, I'm revisiting this now and I think that although a ScriptNum is signed, there is a special case in the evaluation of the CLTV opcode that uses up to a 5-byte ScriptNum when interpreting the lockTime bytes.

dcrd: https://github.com/decred/dcrd/blob/d41cb608bfacf91cd36a28108211e9a0096fc282/txscript/opcode.go#L952-L958
bitcoind: https://github.com/bitcoin/bitcoin/blob/f227e153e80c8c50c30d76e1ac638d7206c7ff61/src/script/interpreter.cpp#L531-L545

I'm not sure if that means it is still a 4-byte lockTime in the Script, but this workaround exits to allow values up to the year 2106, or if it can be up to 5 bytes in the serialized script...

I'm afraid we may actually be doing it wrong by assuming 4 bytes only in ExtractSwapDetails (as well as in this PR).

Ugh, yeah, when we get the bytes for the lockTime data push from tokenizer.Data(), we should be feeding that to txscript.MakeScriptNum(data, 5)... where that 5 is actually a const at txscript.CltvMaxScriptNumLen.
Actually a relief that it's not a 2038 problem, but we've oversimplified by requiring 4 bytes only rather than up to 5.

Been making these annoying CLTV lock time changes. Will push shortly, but in summary, the redeeming transaction's lockTime is a 32-byte unsigned integer, which limits the CLTV lock time value. However, the fact that the ScriptNum is a signed type does not actually limit to 2038 (2^31-1) because there's an exception that allows a 5-byte ScriptNum for the CLTV op code. So, we use a 4 or 5 byte ScriptNum, as required, but we still limit the value to 2^32-1 (sometime in 2106). The little endian Scriptnum(x).Bytes() looks like ffffffff00 (5 bytes so that we can go up to 2^32-1 instead of just 2^31-1).

So that stackexchange is actually sorta relevant because it limits what CLTV lock time values are usable.

The 2038 issue is now resolved in 69d2394

client/asset/dcr/dcr.go

buck54321 · 2022-09-23T00:26:51Z

client/asset/dcr/dcr.go

+		txFee := uint64(baseSize+size+unspent.input.Size()) * feeRate
+		return sum+toAtoms(unspent.rpc.Amount) >= amt+txFee


If we want to ensure that the change output is not dust, we would need to add the minimum output amount here as well.

I may be misunderstanding, but after fund and addInputCoins, we do signTxAndAddChange, which omits the change if it is dust.

client/asset/interface.go

buck54321 · 2022-09-23T00:43:50Z

client/asset/interface.go

+	// ContractLockTimeExpired returns true if the specified contract's locktime
+	// has expired, making it possible to issue a Refund. The contract expiry
+	// time is also returned, but reaching this time does not necessarily mean
+	// the contract can be refunded since assets have different rules to satisfy
+	// the lock. For example, in Bitcoin the median of the last 11 blocks must
+	// be past the expiry time, not the current time.
+	ContractLockTimeExpired(ctx context.Context, contract dex.Bytes) (bool, time.Time, error)


Is it possible to get rid of this and just use LocktimeExpired? Core should always know the locktime, right?

Core should always know the locktime, right?

Not really. For the counterparty contracts, the AuditInfo includes expiry, but we discard that after ensuring it is at least after match time plus expected lock duration for that side.

For our own, we do compute a LockTime from match time and the lockTimeTaker/lockTimeMaker, but... it's also in held in the contract data (and thus db). I dunno, we could jump through hoops to track this in Core, but I don't see a strong reason.

All around, I think it makes the most sense to keep this existing method signature that works on the contract data bytes.

client/asset/interface.go

This formalizes the bond transaction structure for Decred in the common dex/networks/dcr package shared by client and server. Specifically, this adds the functions: - MakeBondScript for constructing a versioned bond output redeem script given a lock time and pubkey hash. - RefundBondScript for creating a signature script for spending a bond output given the signature and public key. - ExtractBondDetailsV0 for validating a bond redeem script and and returning the encoded lockTime and pubkey hash. - ExtractBondCommitDataV0 for parsing a v0 bond commitment output script, which is an OP_RETURN with the details of the bond.

This adds the client/asset.Bonder interface for creating, broadcasting, and refunding fidelity bonds. This updates the client/asset.Wallet interface with separate ContractLockTimeExpired and LockTimeExpired methods, where the former operates on the "contract data" such as an atomic swap redeem script, and the latter checks a time.Time. ContractLockTimeExpired is the existing method used by the atomic swap code. client/asset/dcr: implement asset.Bonder for Decred Decred is the initial Bonder asset. This implements the MakeBondTx, RefundBond, and SendTransaction methods.

This adds the required bond transaction inspection functions for the server to support Decred fidelity bonds. In particular, this adds ParseBondTx for validating of a serialized bond transaction.

chappjc · 2022-10-16T20:13:52Z

Clearing the way for the rest of the bond work, but I do not believe the 2038 resolution discussed in #1818 (comment) or the refactored ExtractBondDetailsV0 has been reviewed.

This was referenced Aug 24, 2022

fidelity bonds foundation: integration, client db #1820

Merged

fidelity bonds foundation - DCR only, no frontend, manual bond replacement #1480

Closed

chappjc force-pushed the bond-assets branch 2 times, most recently from bfccf7b to d5b6b37 Compare August 25, 2022 15:11

chappjc marked this pull request as ready for review August 25, 2022 15:49

martonp reviewed Aug 27, 2022

View reviewed changes

client/asset/dcr/dcr.go Show resolved Hide resolved

chappjc force-pushed the bond-assets branch 2 times, most recently from b79404f to 1de51b0 Compare September 1, 2022 23:58

JoeGruffins reviewed Sep 5, 2022

View reviewed changes

chappjc added this to the 0.6/1.0 milestone Sep 6, 2022

buck54321 reviewed Sep 7, 2022

View reviewed changes

chappjc force-pushed the bond-assets branch from 1de51b0 to 2e12694 Compare September 7, 2022 17:07

JoeGruffins approved these changes Sep 8, 2022

View reviewed changes

chappjc force-pushed the bond-assets branch from 0caec6d to 642bc54 Compare September 16, 2022 14:40

buck54321 reviewed Sep 23, 2022

View reviewed changes

chappjc force-pushed the bond-assets branch from 642bc54 to c7b34f5 Compare September 29, 2022 20:39

chappjc force-pushed the bond-assets branch from c7b34f5 to 69d2394 Compare October 12, 2022 15:57

chappjc mentioned this pull request Oct 12, 2022

fidelity bonds foundation: comms, account tiering, server tracking #1819

Merged

chappjc added 3 commits October 16, 2022 15:10

server/asset/dcr: bond validation

3150c8f

This adds the required bond transaction inspection functions for the server to support Decred fidelity bonds. In particular, this adds ParseBondTx for validating of a serialized bond transaction.

chappjc force-pushed the bond-assets branch from 69d2394 to 3150c8f Compare October 16, 2022 20:10

chappjc merged commit 3ae066b into decred:master Oct 16, 2022

chappjc deleted the bond-assets branch October 16, 2022 20:17

chappjc restored the bond-assets branch October 16, 2022 20:17

chappjc added the bonds fidelity bonds label Feb 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fidelity bonds foundation: define bond tx structure, create and parse #1818

fidelity bonds foundation: define bond tx structure, create and parse #1818

chappjc commented Aug 24, 2022 •

edited

Loading

JoeGruffins commented Sep 5, 2022

buck54321 Sep 7, 2022

chappjc Sep 7, 2022

buck54321 Sep 7, 2022

buck54321 Sep 7, 2022 •

edited

Loading

chappjc Sep 7, 2022 •

edited

Loading

chappjc commented Sep 8, 2022

buck54321 left a comment

buck54321 Sep 22, 2022

chappjc Sep 26, 2022

chappjc Sep 26, 2022 •

edited

Loading

chappjc Sep 29, 2022 •

edited

Loading

chappjc Oct 12, 2022

buck54321 Sep 23, 2022

chappjc Sep 29, 2022

buck54321 Sep 23, 2022

chappjc Sep 29, 2022 •

edited

Loading

chappjc commented Oct 16, 2022

		txFee := uint64(baseSize+size+unspent.input.Size()) * feeRate
		return sum+toAtoms(unspent.rpc.Amount) >= amt+txFee

fidelity bonds foundation: define bond tx structure, create and parse #1818

fidelity bonds foundation: define bond tx structure, create and parse #1818

Conversation

chappjc commented Aug 24, 2022 • edited Loading

Background from #1480:

Bond Transaction Structure (version 0)

Time-locked Bond (output 0)

DEX Account commitment (output 1)

Open Questions

JoeGruffins commented Sep 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buck54321 Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

chappjc Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

chappjc commented Sep 8, 2022

buck54321 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Sep 26, 2022 • edited Loading

Choose a reason for hiding this comment

chappjc Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

chappjc commented Oct 16, 2022

chappjc commented Aug 24, 2022 •

edited

Loading

buck54321 Sep 7, 2022 •

edited

Loading

chappjc Sep 7, 2022 •

edited

Loading

chappjc Sep 26, 2022 •

edited

Loading

chappjc Sep 29, 2022 •

edited

Loading

chappjc Sep 29, 2022 •

edited

Loading