-
Notifications
You must be signed in to change notification settings - Fork 92
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
multi: send raw tx data in audit and match_status requests #804
Conversation
I'll note that not only is this needed for SPV clients, it will make the process more robust in general as slow or generally poor propagation of transactions on the network is less of an issue. I know that requires rebroadcasting the txn at each point, which is not in this PR, but this obviously sets up for that. |
client/asset/interface.go
Outdated
@@ -118,7 +118,7 @@ type Wallet interface { | |||
// during a swap. If the coin cannot be found for the coin ID, the | |||
// ExchangeWallet should return CoinNotFoundError. This enables the client | |||
// to properly handle network latency. | |||
AuditContract(coinID, contract dex.Bytes) (AuditInfo, error) | |||
AuditContract(coinID, contract, txData dex.Bytes) (AuditInfo, error) |
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.
Maybe comment on how txData will be used. I'm actually still wondering how exactly we will robustly get confirmation that the rebroadcast we are planning "succeeded". Presumably we'll decode the txn, validate it (against the provided contract script), and then we'll try to detect sendrawtransaction errors that just indicate the txn already exists, but I'm a little concerned there will be a variety of errors that look different but all stem from the tx already being known, and we may end up declaring audit failures improperly. I guess we'll cross that bridge next.
client/core/core.go
Outdated
counterTxData := metaData.Proof.CounterTxData | ||
if len(counterTxData) == 0 { | ||
match.swapErr = fmt.Errorf("missing counter-tx data, order %s, match %s", tracker.ID(), match.id) | ||
notifyErr("Match status error", "Match %s for order %s is in state %s, but has no maker swap tx data.", dbMatch.Side, tracker.token(), dbMatch.Status) | ||
continue |
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.
I got the impression from the other changes that this PR would make the client able to work with a server that does not yet provide the CounterTxData
. https://github.com/decred/dcrdex/pull/804/files#diff-8962b4c526755f0f98ed9f46ae05827e8edbc43ed5671ea4a2624bdf1617496cR2029 ((*trackedTrade).auditContract
). Should this be an error+self-revoke and continue to next match, as you have it, or just pass nil to AuditContract
and try to work with it? Ofc AuditContract
doesn't use it yet so it will be fine, but I figure this will be the upgrade plan where the server is behind clients.
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.
Yeah. I got a little overzealous here. This is going to be tricky without versioning, because we can't allow the use of SPV wallets on pre-tx-data servers.
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.
First pass, looks good overall. Mostly nitpicking below:
@@ -291,7 +292,8 @@ func (p *MatchProof) Encode() []byte { | |||
AddData(auth.RedemptionSig). | |||
AddData(uint64Bytes(auth.RedemptionStamp)). | |||
AddData(srvRevoked). | |||
AddData(selfRevoked) | |||
AddData(selfRevoked). | |||
AddData(p.CounterTxData) |
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.
CounterTx
could be ambiguous, other party contract tx or other party redemption tx?
Suggest CounterContractRawTx
server/swap/swap.go
Outdated
@@ -1357,6 +1357,7 @@ func (s *Swapper) processInit(msg *msgjson.Message, params *msgjson.Init, stepIn | |||
Time: uint64(swapTimeMs), | |||
CoinID: params.CoinID, | |||
Contract: params.Contract, | |||
TxData: contract.TxData(), |
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.
Do you think ultimately the client can provide the raw tx in the init request to the server as well? This PR hits the other half of it great though -- supplementing the server-originating audit and match_status requests, so that that client may rebroadcast it in subsequent work.
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.
With our current protocol and assets, we have to fetch the transaction anyway as part of validation. I like the idea of the client providing the details from a standpoint of limiting the server's role as a source of on-chain data, but it's net effect right now would just be more bandwidth. I could be convinced though, if you're adamant.
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.
Nope, just a thought.
server/auth/auth.go
Outdated
var txData, txDataCoin []byte | ||
var assetID uint32 | ||
switch { | ||
case status.IsTaker && status.Status == order.MakerSwapCast: // && !status.IsMaker ? |
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.
// && !status.IsMaker ?
Is a question? Can't be both maker and taker right?
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.
Can be when trading with self. I've fallen into that mistaken assumption before too. Def. have to consider that situation.
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.
Should uncomment this check then to be safe.
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.
How to handle it correctly depends on the status of the match and how that changes the semantics of TxData
. This pertains to the comment in my review:
Regarding the TxData in MatchStatusResult, either we document very clearly that TxData can refer to different transactions depending on the match status (esp. important if user is both maker and taker) or we change to two fields, MakerTxData and TakerTxData, to avoid any ambiguity. What do you think?
The case
here also requires MakerSwapCast
as the match status, and if taker, that is when you want this particular tx data (the maker's raw swap tx)... and whether it's a self-swap or not doesn't seem to matter, but I could be mistaken.
Since we're all talking about it, I'm suspecting more strongly now that the 'match_status'
route's txdata field could use more clarity, even if that means making two separate fields and even setting them when not needed at the current status or recipient if that is clearer.
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.
To clarify, I think the code is right as is. And I don't think sending lots of extra data and when it's not needed is absolutely necessary, but it could make things clearer. Documenting the msgjson.MatchStatusResult extra clearly w.r.t. this field would also be cool.
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.
I'm for having 2 fields for {M,T}akerTxData
, even though both need not be set at the same time, similar to the existing {M,T}akerSwap
, {M,T}akerContract
and {M,T}akerRedeem
fields.
client/core/status.go
Outdated
// len(srvData.TxData) == 0 { | ||
// err = fmt.Errorf("Server is reporting a match in TakerSwapCast with no tx data. %s", logID) | ||
// return | ||
// } | ||
|
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.
Why is this one commented out?
I think this was commented somewhere else, but I guess the client will need to check that the server's version is compatible with itself. An older server would not include the TxData
.
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 PR is just a part of a few steps to this work (next is presumably using the TxData with bcast), but it seems to me that backward compatibility regarding this TxData field is straightforward for both client and server.
- If the TxData is omitted/empty, the client (asset backend) should take the legacy approach of searching for the txn
- If the TxData is present, take the new approach of broadcasting it
I don't believe any versioning is required to handle this elegantly and without any hacks.
As such, I don't think this commented code will ever be uncommented given the legacy handling I'm suggesting if it is len 0.
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.
Some minor cleanup on client db upgrade code.
Regarding the TxData
in MatchStatusResult
, either we document very clearly that TxData can refer to different transactions depending on the match status (esp. important if user is both maker and taker) or we change to two fields, MakerTxData
and TakerTxData
, to avoid any ambiguity. What do you think?
server/auth/auth.go
Outdated
var txData, txDataCoin []byte | ||
var assetID uint32 | ||
switch { | ||
case status.IsTaker && status.Status == order.MakerSwapCast: // && !status.IsMaker ? |
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.
Can be when trading with self. I've fallen into that mistaken assumption before too. Def. have to consider that situation.
RefundCoin: pushes[8], | ||
Script: pushes[0], | ||
CounterContract: pushes[1], | ||
CounterTxData: pushes[21], |
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.
Why not pushes[2]
to maintain the order and aid readability.
In decodeMatchProof_v1
, instead of appending the nil []byte
, can insert:
pushes = append(pushes, nil)
copy(pushes[3:], pushes[2:])
pushes[2] = nil
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.
I don't really see how this would improve readability. Simpler is better in these upgrades, and changing the index of every read of pushes
for index > 1 is more error-prone.
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 works for sure, but the readability improvements I mean is having the order of the fields match the order of data in the pushes. So last field in the struct should write to / read from the last data in pushes
. It'd become a bit of a eye turner if for example, the first struct field is populated with the 10th data in pushes
, the second field is populated with the data at index 3 and the last field is populated with index 1.
What I'm recommending for clarity is:
Script: pushes[0],
CounterContract: pushes[1],
CounterTxData: pushes[2],
SecretHash: pushes[3],
Secret: pushes[4],
MakerSwap: pushes[5],
instead of
Script: pushes[0],
CounterContract: pushes[1],
CounterTxData: pushes[21],
SecretHash: pushes[2],
Secret: pushes[3],
MakerSwap: pushes[4],
server/asset/common.go
Outdated
@@ -26,6 +26,8 @@ type Backend interface { | |||
// the blockchain immediately. The redeem script is required in order to | |||
// calculate sigScript length and verify pubkeys. | |||
Contract(coinID []byte, redeemScript []byte) (Contract, error) | |||
// TxData fetches the same data returned by a Contract's TxData method. |
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.
Looks like you missed https://github.com/decred/dcrdex/pull/804/files#r557809395
// TxData fetches the same data returned by a Contract's TxData method. | |
// TxData fetches the raw transaction data of the specified coin. |
server/auth/auth.go
Outdated
var txData, txDataCoin []byte | ||
var assetID uint32 | ||
switch { | ||
case status.IsTaker && status.Status == order.MakerSwapCast: // && !status.IsMaker ? |
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.
Should uncomment this check then to be safe.
client/db/bolt/upgrades_test.go
Outdated
// The indices of the archives here in outdatedDBs should match the first | ||
// eligible validator for the database. | ||
var outdatedDBs = []string{ | ||
"v0.db.gz", "v1.db.gz", | ||
} |
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.
Update doc, add "v2.db.gz"
or better still make TestUpgradeDB
use dbUpgradeTests
but only the filename
property to avoid having to define filenames twice.
client/core/core.go
Outdated
// TODO: We will need careful handling here. To be clear, the | ||
// client should reject orders at the funding stage when the | ||
// server doesn't signal compatibility with the requisite | ||
// protocols. e.g spv-compatibility. So if we version the api to |
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.
I don't think clients need to reject orders when a server does not provide txdata. If they allow orders still, what happens is that they simply can't find the counter contract until it is mined. Even in full node, a client cannot act until the counter contract is mined. Also, in full node, if a client cannot locate the counter contract tx, it doesn't do anything but wait until the server revokes the match, iirc. Add as @chappjc noted in chat, a full node wallet can fail to locate the counter contract tx and have to wait till it is mined if a user restarts their nodes and the mempool loses the txn.
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.
If the above holds true, then the addition of txdata isn't really something done to aid spv-compatibility.
As noted above whether spv or not, clients will wait for txs to be mined before acting and only abandon matches when the server revokes them, not if they cannot find a particular tx.
The real advantage to adding txdata imo, is to ensure that counter parties play their parts by broadcasting valid contracts, with receiving clients having the ability to also broadcast as a fail safe. With this addition, clients (with spv or full node wallets) can abandon matches earlier if a provided tx is invalid or fails audit checks such as address and amount. This is why I recommended in a previous review to not allude the introduction of txdata to spv compatibility - it started that way but I believe things have taken a turn since the change to allow audits run until the contract tx is mined or the server revokes the match.
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.
I'm not quite sure what you're getting at here. If we weren't doing SPV, we wouldn't be making these changes.
The real advantage to adding txdata imo, is to ensure that counter parties play their parts by broadcasting valid contracts, with receiving clients having the ability to also broadcast as a fail safe.
We already do this verification without tx data from the server though. We only need the tx data so that SPV clients can do the same validation immediately, since they won't have access to the mempool data. The fail-safe definition strikes me wrong too. So what has failed, and why if something has failed, are we progressing the trade for the counter-party? I'm not convinced that broadcasting the counter-party's transaction is a route we want to take as a remedial action.
And even though we are no longer using broadcast timeout to secure the client, I'm not sure it's an option we should throw away. We are not very well protected against a malicious server right now.
With this addition, clients (with spv or full node wallets) can abandon matches earlier if a provided tx is invalid or fails audit checks such as address and amount.
Something we can already do for full node-backed wallets. We only need the tx data so that spv clients can do it too.
I do acknowledge that with our new leniency on broadcast timeouts, having the transaction data before mining gives us little advantage. But without tx data our spv clients will be more vulnerable to attacks from malicious servers.
Which brings us back to
I don't think clients need to reject orders when a server does not provide txdata.
This feels like a new spin on the same conversation we've had many times. We do not want our clients sitting in the dark. A malicious server could effectively lock up the funds of every spv client with just a random hash generator.
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.
We do not want our clients sitting in the dark. A malicious server could effectively lock up the funds of every spv client with just a random hash generator.
But this problem already exists, it's not particular to spv wallets. If a server sends random hashes and a client is unable to locate the tx, funds will remain locked until the server revokes the match. On that basis, sending txdata to combat this issue doesn't only apply to spv wallets.
We already do this verification without tx data from the server though. We only need the tx data so that SPV clients can do the same validation immediately, since they won't have access to the mempool data.
Full node-backed wallets are not always currently able to audit contracts since they may not find the tx in their mempool for different possible reasons such as the user restarting wallet node and dexc after counter party has broadcasted their tx. When this happens, the user has to wait till the contract is mined to perform any validation. Even full node-backed wallets. Sending txdata for full node-backed wallet clients gives clients the ability to perform audits on the provided txdata without needing to find the tx in the blockchain first. Agreed, I expect it'll be less common for full node-backed wallets to not find contract txs in their mempool, but it's a very realistic possibility.
I do acknowledge that with our new leniency on broadcast timeouts, having the transaction data before mining gives us little advantage. But without tx data our spv clients will be more vulnerable to attacks from malicious servers.
The only protection full node wallets currently have over spv wallets is they can perform audit and abandon a match sooner if the contract tx doesn't meet expectations - and this advantage only exists if the full node wallet does find the contract tx before it is mined, which as has been established, may not always happen. On this note, sending txdata offers almost just as much extra advantage for full node wallet as it does for spv wallets - provide the ability to audit contracts regardless of whether they can be found in mempool or not.
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.
I think the meat of my point is:
As noted above whether spv or not, clients will wait for txs to be mined before acting and only abandon matches when the server revokes them, not if they cannot find a particular tx.
This is the key reason why it makes very little difference whether a wallet can find a tx in mempool or not, whether spv or full node-backed. If we say spv wallets should not trade if they won't get txdata from the server, then we should say full node-backed wallets should abandon matches if they can't find a tx in mempool. If we allow full node wallets to wait till the tx is mined or the server revokes the match, why aren't we allowing svp wallets to do the same?
The only scenario where I think full node wallets have an advantage is the combination of the following:
- the server sends a hash of a real transaction but that does not meet audit requirements
- the wallet finds the tx (mined or unmined) and abandons the match without waiting for the server's revoke.
If that were to happen to an spv wallet that does not have the txdata, the client would not abandon the match until the server revokes it or the wallet finds the mined tx and then performs the audit checks.
Still, if a server really wants to be malicious and blind-side spv wallets, the server can provide a valid txdata that passes audit checks but is a double spend and the client will not know even if the client broadcasts the txdata because spv clients do not really have any validation that a rawtx is accepted into the blockchain until it is mined. So again, they have to wait.
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.
That all makes sense, and I agree. Was it just the mention of spv in the TODO that you were concerned with?
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.
Not that we want more options and switches, but I wonder if it would be helpful to have an option for SPV-only clients to "reject orders at the funding stage when the server doesn't signal compatibility". I think we'd have to evaluate the UX for SPV-only clients is in the absence of this data. That is, will the confusion and suspense waiting for the initial confirm be tolerable?
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.
Even in the absence of the transaction data, we can still link to the transaction in a block explorer, which will presumably have it in mempool.
We could almost use dcrdata as an spv mempool, but let's not.
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.
@Wisdom @itswisdomagain I'm just going to delete these TODOs and comments for now, if nobody objects.
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.
You tagged the wrong username 😉 . Life would probably be easier if my username everywhere was just [at]wisdom but we move.
As per off-github conversation. To accommodate SPV clients, who don't have a mempool and so are in the dark until a contract's transaction is mined, we are going to provide the serialized swap transactions to the clients. The server sends sends the data in the
msgjson.Audit
payload. Server also sends the info in themsgjson.MatchStatusResult
, but only for status/side combos for which it is relevant.The wallet does not make use of the info yet, but it does receive the data though the modified
(Wallet).AuditContract
method. There are some other changes that will be needed in the wayCore
handles the audit too, but I think it'll be easier to handle that during work on the wallet.This work is closely related to #788.