client: add support for dcr spv wallets #788

itswisdomagain · 2020-10-27T04:41:35Z

The following is a summary of changes made to support dcr spv wallets:

Validate and (re-)broadcast txData when auditing contracts.
Replace getblockchaininfo with syncstatus to determine sync status.
In spv mode, use block cfilters to find tx confirmations if gettxout and gettransaction can't find a tx. Both rpcs only returns results for wallet unspent outputs.
Re-audit contracts after they receive the required confirmations but before sending counter-swap (if taker) and before sending redeem (if maker).

The dcr harness is modified to use SPV for the second trading wallet to facilitate testing.

itswisdomagain · 2021-07-31T15:25:36Z

The commit message on 9b51633 provides my reasoning for making clients re-audit contracts before acting.

Copied below for posterity:

There is a very slight possibility that contract outputs that pass
audit while in mempool or in some cases before they are broadcasted
(rawtx audit) may differ from the output later observed in a block
for the same coin.

This risk is higher for spv wallets that will mostly only perform
audits on rawtxs before broadcasting the txs, without a guarantee
that the tx is accepted to the mempool. A malicious actor could
broadcast a different tx with same hash (theoretically possible)
but with a different output at the expected vout index.

There is risk of funds if clients only later check that the hash for
the earlier-audited tx is found in a block and proceed to send their
counter swap or expose their contract secret via a redemption. This
commit aims to mitigate that risk by repeating contract audits after
the initial tx hash is observed on the blockchain, ensuring that the
tx now observed on the blockchain is as desired.

The risk of fund loss I describe above feels less real than when I first thought about it. I've made that change a separate commit so I can easily remove it if the risk identified is not realistic.

chappjc · 2021-07-31T15:28:13Z

The risk of fund loss I describe above feels less real than when I first thought about it. I've made that change a separate commit so I can easily remove it if the risk identified is not realistic.

Yeah barring hash collisions, I don't think the txns are malleable in any way that is significant for the counterparty, but checking again sounds just fine to me.

itswisdomagain · 2021-07-31T15:36:24Z

I plan to add some new tests but code's set for review in the meantime.
All current tests are passing, including trade_simnet_tests and dcr simnet tests.

itswisdomagain

Oops.

client/asset/dcr/dcr.go

chappjc

Partial review. I'm still going through externaltx.go, but I wanted to point out the block header thing.

client/asset/dcr/dcr.go

dex/testing/dcr/harness.sh

chappjc

Code mostly makes sense, but there are somethings I think you can help me understand, plus I need to actually fire up dcrwallet with --spv to test.

client/asset/dcr/externaltx.go

client/asset/dcr/dcr.go

client/asset/btc/btc.go

client/asset/dcr/externaltx.go

- Don't error for missing dcrdjsonrpcapi, instead set wallet.spvMode=true. - Replace getblockchaininfo with syncstatus to determine sync status.

There is a very slight possibility that contract outputs that pass audit while in mempool or in some cases before they are broadcasted (rawtx audit) may differ from the output later observed in a block for the same coin. This risk is higher for spv wallets that will mostly only perform audits on rawtxs before broadcasting the txs, without a guarantee that the tx is accepted to the mempool. A malicious actor could broadcast a different tx with same hash (theoretically possible) but with a different output at the expected vout index. There is risk of funds if clients only later check that the hash for the earlier-audited tx is found in a block and proceed to send their counter swap or expose their contract secret via a redemption. This commit aims to mitigate that risk by repeating contract audits after the initial tx hash is observed on the blockchain, ensuring that the tx now observed on the blockchain is as desired.

The getrawtransaction rpc requires txindex to be enabled on full nodes but clients may run full nodes without enabling txindex as it is not a requirement. Use gettransaction where possible instead of getrawtransaction to avoid errors when clients use full nodes without txindex enabled.

fix dcr harness spv wallets startup issues fix SwapConfirmations error for wallet contracts fix output spent check bug and repaired log messages

buck54321

Thanks for the follow through. This seems solid now.

I'll be following up quickly to propose some small changes to the Wallet interface.

buck54321 mentioned this pull request Oct 31, 2020

multi: send raw tx data in audit and match_status requests #804

Merged

itswisdomagain marked this pull request as draft December 29, 2020 11:01

itswisdomagain mentioned this pull request Mar 6, 2021

multi: add blockcache to client/asset/dcr #1004

Closed

itswisdomagain force-pushed the dcr-spv branch from 825a61c to f51802f Compare March 11, 2021 18:15

chappjc added this to the 0.3 milestone Apr 7, 2021

itswisdomagain force-pushed the dcr-spv branch 2 times, most recently from a669641 to 81a850c Compare June 1, 2021 09:47

chappjc self-requested a review July 15, 2021 01:36

itswisdomagain force-pushed the dcr-spv branch 5 times, most recently from 4673470 to 7862ae4 Compare July 31, 2021 15:10

itswisdomagain changed the title ~~client/asset/dcr: changes to support spv wallets~~ client: add support for dcr spv wallets Jul 31, 2021

itswisdomagain force-pushed the dcr-spv branch from 7862ae4 to d51b6b1 Compare July 31, 2021 15:34

itswisdomagain marked this pull request as ready for review July 31, 2021 15:35

chappjc added the SPV label Aug 2, 2021

chappjc modified the milestones: 0.3, 0.4 Aug 3, 2021

chappjc mentioned this pull request Aug 4, 2021

client/asset/dcr: refactor findRedemptionsInBlockRange to use cfilters #954

Merged

itswisdomagain commented Aug 24, 2021

View reviewed changes

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

chappjc requested changes Aug 24, 2021

View reviewed changes

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

dex/testing/dcr/harness.sh Show resolved Hide resolved

itswisdomagain force-pushed the dcr-spv branch from af92dec to 1f6423c Compare August 24, 2021 17:13

chappjc requested changes Aug 24, 2021

View reviewed changes

chappjc mentioned this pull request Sep 14, 2021

client: prepare interfaces for bitcoin spv #1089

Merged

itswisdomagain force-pushed the dcr-spv branch from 1f6423c to 6ab26fc Compare October 5, 2021 23:35

chappjc reviewed Oct 6, 2021

View reviewed changes

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

chappjc reviewed Nov 7, 2021

View reviewed changes

client/asset/btc/btc.go Outdated Show resolved Hide resolved

chappjc reviewed Nov 8, 2021

View reviewed changes

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

itswisdomagain added 21 commits November 8, 2021 21:43

client/asset/dcr: validate and broadcast txData in AuditContract()

0451243

client/asset/dcr: add basic support for spv wallets

8fa63e5

- Don't error for missing dcrdjsonrpcapi, instead set wallet.spvMode=true. - Replace getblockchaininfo with syncstatus to determine sync status.

dcr harness: use spv for trading2 wallet

c3d9007

client/asset/dcr: use cfilters to find txout confirmations

af46a25

client/asset/dcr.AuditContract: handle txdata broadcast errors

2355326

fix trade_simnet_test duplicate balance change tracking

3c11408

rebase fix, partial chapp review corrections

b0749cf

rework swap confirmations code

7e211c8

more review fixes

ad2e26c

fix dcr harness spv wallets startup issues fix SwapConfirmations error for wallet contracts fix output spent check bug and repaired log messages

pass feeratesuggestion to PayFee, fix txoutspender loop

742432e

use wallet+external output lookup for audit, refund and swapconfs

19fd34a

client/asset/dcr: check walletInfo.SPV and handle rpcclient reconnects

e569f68

no txdata for post-confirmation repeat audit

eb24720

trade_simnet_test changes for better btc spv wallet support

785973a

fix wallet output check for spv wallets

5a53e29

bump dcr rpc requiredWalletVersion

ab0d0bd

rework dcr.lookupTxOut{WithBlockFilters}, rename dcr.Wallet.GetTxOut

d798528

buck's review, btc audit txdata only

5f884a5

chapp review

73ca58c

buck54321 approved these changes Nov 8, 2021

View reviewed changes

itswisdomagain force-pushed the dcr-spv branch from 9ae9f12 to a991e82 Compare November 8, 2021 21:03

cleanup

b20b179

itswisdomagain force-pushed the dcr-spv branch from a991e82 to b20b179 Compare November 8, 2021 21:10

chappjc approved these changes Nov 8, 2021

View reviewed changes

chappjc merged commit fe1995a into decred:master Nov 8, 2021

itswisdomagain deleted the dcr-spv branch November 8, 2021 21:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client: add support for dcr spv wallets #788

client: add support for dcr spv wallets #788

itswisdomagain commented Oct 27, 2020 •

edited

itswisdomagain commented Jul 31, 2021

chappjc commented Jul 31, 2021 •

edited

itswisdomagain commented Jul 31, 2021 •

edited

itswisdomagain left a comment

chappjc left a comment

chappjc left a comment

buck54321 left a comment •

edited

client: add support for dcr spv wallets #788

client: add support for dcr spv wallets #788

Conversation

itswisdomagain commented Oct 27, 2020 • edited

itswisdomagain commented Jul 31, 2021

chappjc commented Jul 31, 2021 • edited

itswisdomagain commented Jul 31, 2021 • edited

itswisdomagain left a comment

Choose a reason for hiding this comment

chappjc left a comment

Choose a reason for hiding this comment

chappjc left a comment

Choose a reason for hiding this comment

buck54321 left a comment • edited

Choose a reason for hiding this comment

itswisdomagain commented Oct 27, 2020 •

edited

chappjc commented Jul 31, 2021 •

edited

itswisdomagain commented Jul 31, 2021 •

edited

buck54321 left a comment •

edited