client: bitcoin spv #1230
client: bitcoin spv #1230
Conversation
|
I still have concerns with potentially performing long unnecessary scans. Let's say the taker's swap is sitting in mempool due to fee volatility. Every 5 seconds (the interval of the We could avoid these scans if we had some kind of checkpoint system. A way to say "we already looked for this output, and didn't see it through block X, so restart the scan there (if not orphaned)". Sorta like what we do with Update: solution in bbd1669 |
| type CreateWalletParams struct { | ||
| Type string | ||
| Seed []byte | ||
| Pass []byte |
There was a problem hiding this comment.
Shouldn't the password be a string since a string is passed to wallet.Unlock?
There was a problem hiding this comment.
Hmm. Good point. I'm tempted to go the other way though, and change the (Wallet).Unlock method to accept a []byte.
There was a problem hiding this comment.
Won't that require a bunch of changes for non seeded wallets? The geth node takes a string to lock/unlock the private key as well.
There was a problem hiding this comment.
It will require changing existing implementations, but it's not a big job. The thing about string passwords is that once we're done with them, we're at the mercy of the garbage collector to get rid of the sensitive data. But we can zero a []byte. We even have a type for it, encode.PassBytes. It's true that eth will still be converting to string. So will dcr and btc-rpc, but we can now avoid strings for btc-spv, and we should take the opportunity to make things more secure.
There was a problem hiding this comment.
I thought we had a dex.Bytes type for passwords.
| // 3. We don't see a mempool. We're blind to new transactions until they are | ||
| // mined. This requires special handling by the caller. We've been | ||
| // anticipating this, so Core and Swapper are permissive of missing acks for | ||
| // audit requests. |
There was a problem hiding this comment.
although Core responds to the audit request based on the txData, right? This is a note to self to check
There was a problem hiding this comment.
We're actually not yet, but I think that's where we're headed.
client/asset/btc/spv.go
Outdated
| if len(msgTx.TxOut) < int(vout+1) { | ||
| return nil, 0, fmt.Errorf("wallet transaction %s found, but not enough outputs for vout %d", txHash, vout) | ||
| } | ||
| return msgTx.TxOut[vout], confs, nil |
There was a problem hiding this comment.
In this case where walletTransaction found it, it returns a non-nil txout regardless of spent status, which is at odds with the docs for getTxOut. Only for WalletTransactionNotFound where it calls scanFilters does it emulate gettxout returning nil when spent.
Basically, the behavior matches for non-wallet txns, but for wallet txns, it will never appear spent. Are we ok with that?
If it's a wallet output too, could check txDetails.Credits[].Spent? If its not in the Credits slice, then it's a non-wallet-output (right?) like own swap, and then I don't know... just return as if not spent I guess?
client/asset/btc/spv.go
Outdated
| w.log.Debugf("A spending input was found during the scan but the output " + | ||
| "itself wasn't found. Was the startBlockHeight early enough?") |
There was a problem hiding this comment.
Warnf? What's the severity and resolution if this happens? clear the checkpoint so it starts over from match time?
There was a problem hiding this comment.
Warnf seems appropriate. We don't store a *scanCheckpoint in this case.
This place this ends up affecting us is in SwapConfirmation, we return confs = 0 (which is wrong) and spent = true. In situations where we're looking for the counter-party's swap, we revoke if it's found to be spent, without inspecting the confs, so we're all good there. When it's our output, we'll know the block and won't end up here, but even if we did, we just end up sending out some inaccurate Data-severity notifications to the UI until the match progresses.
This comment has been minimized.
This comment has been minimized.
|
Got an intermittent EDIT: Appears resolved with latest commits. |
This comment has been minimized.
This comment has been minimized.
|
Here's a weird one that's not related to logging at all, all inside neutrino and btcd code.. EDIT: looks to be a flaw with the btcd |
| if coinNotFound { | ||
| return nil, asset.CoinNotFoundError | ||
| } |
There was a problem hiding this comment.
Ok, so more actual auditing of the txData, but not totally passing just yet if it's not found on the network...
And if we decided to just pass the audit from txData and ack to server, we could just remove this check?
Any reason to delay on that change, just one step at a time? I think @itswisdomagain was proposing to simply audit a second time when the counterparty contract is confirmed, before broadcasting anything of our own.
There was a problem hiding this comment.
I think maybe we should just audit (and re-broadcast?) the tx data in AuditContract without finding the output on-chain. We'll ack based on that, and then worry about finding the transaction with SwapConfirmation and be more permissive of CoinNotFound error there. I don't personally see a lot of value in double checking that the on-chain contract validates the same as the txData, but it looks like @itswisdomagain thought it was a good idea. What do you think @itswisdomagain? Can we get rid of a coin search in AuditContract altogether.
There was a problem hiding this comment.
and be more permissive of
CoinNotFounderror there
I guess this is the twist.
I don't personally see a lot of value in double checking that the on-chain contract validates the same as the txData
Felt prudent to me too, but I'm not totally set on that.
There was a problem hiding this comment.
I had some concerns about the possibility of finding an on-chain contract that differs from the txData that was validated, differs enough to have failed audit checks but after further consideration, the possibility became less real to me. Still felt safer to repeat audit before broadcasting a counter-swap or redeeming, but I'm not set on that either.
Wrote my initial thoughts on this topic here: #788 (comment).
This does look like a btcd API design issue that makes the neutrino has a virtually identical If anyone else would care to look at it and give feedback I'd appreciate it. I'm not decided yet if I should raise the issue with the neutrino and/or btc devs. I've pinged @davecgh for his eyes on this. |
|
|
||
| if coinNotFound { | ||
| return nil, asset.CoinNotFoundError | ||
| } |
There was a problem hiding this comment.
This seems wrong. This doesn't change since the last check and there's no positive/non-err return path between there and here. It could return much earlier.
EDIT: Hmm, now I see you are doing a EDIT 2: confirmed that at least some of the test races are because wallet shutdown wasn't waiting on |
| return newError(passwordErr, "cannot set a password on a seeded wallet") | ||
| } | ||
|
|
||
| exists, err := asset.WalletExists(assetID, form.Type, c.assetDataDirectory(assetID), form.Config, c.net) |
There was a problem hiding this comment.
The btc driver implementation of this method (haven't checked others) expects that this method may be called for RPC wallets as well. Is that the case or is that planned?
There was a problem hiding this comment.
Yeah, but I think I'm going to remove this RPC checking capability and just doc that the method is for seeded wallets only. Does that work?
There was a problem hiding this comment.
Fine with me.
Although the seeded vs built-in terminology can be confusing I think because as has been pointed out, most wallets are seeded even the external ones, when really when we say "seeded" we mean created and managed by the dex process. Internally we have solidified the seeded concept though, even for wallets like eth where we're shoehorning the seed because geth has no idea what a seed is for.
Whatever makes sense to you though. These are code docs, not user-facing messages.
There was a problem hiding this comment.
Internally we have solidified the seeded concept though
I generally prefer to use descriptive terms rather than potentially confusing terms which meanings may need to be explained to new/other devs. Yes, the seeded concept has been clarified but docs and variable names should also reflect this clarification so the confusion don't re-arise tomorrow.
buck54321
force-pushed
the
btcspv
branch
2 times, most recently
from
537182e to
725d561
Compare
client/asset/interface.go
Outdated
| @@ -22,6 +22,27 @@ const ( | |||
| ErrUnsupported = dex.ErrorKind("unsupported") | |||
| ) | |||
|
|
|||
| type WalletDefinition struct { | |||
| // If seeded is true, the Setup method will be provided a detereministic | |||
There was a problem hiding this comment.
Create method instead of Setup method.
| // Probably not worth doing. Just let decodeWallet_v0 append the nil and pass it | ||
| // up the chain. |
There was a problem hiding this comment.
I think you should still increment a database version. If one had a wallet update, silently, and tried to use the previous version with the same database, they might not know why it no longer works.
Trying it out, that is going back to master from this pr, there is this log, that doesn't stop anything, but I think upping the db version would and should.
[ERR] CORE: error loading wallets from database: DecodeWallet error: unknown DecodeWallet version 1
And then there is no indication on the UI, it spins for a while, and you can add wallets again, but if you reset the app they don't load again.
| ) | ||
|
|
||
| var ( | ||
| driversMtx sync.RWMutex | ||
| drivers = make(map[uint32]Driver) | ||
| ) | ||
|
|
||
| // CreateWalletParams are the parameters for internal wallet creation. The |
There was a problem hiding this comment.
Sorry to bring this up and doesn't need to be addressed right away or at all even, but this sounds like these parameters are only meant for builtin/internal wallets. If dev wordings in other places are selected to accommodate the possibility of using CreateWallet with external wallets, might as well do so here.
client/asset/driver.go
Outdated
| // CreateWalletParams are the parameters for internal wallet creation. The | ||
| // Settings provided should be the same wallet configuration settings passed to | ||
| // Create. |
There was a problem hiding this comment.
I asked about this in #1225, I do not completely understand these references to Settings. Is my understanding below correct?
The settings provided - the Settings field of this CreateWalletParams
wallet configuration settings passed to ... - the settings passed to OpenWallet?
There was a problem hiding this comment.
I'm assuming this is saying it's the same settings map in WalletConfig, passed to OpenWallet, as you say.
There was a problem hiding this comment.
Yes. Same typo here as there. Sorry about that.
| AvailableWallets: []*asset.WalletDefinition{{ | ||
| Type: walletTypeRPC, | ||
| Tab: "External", | ||
| Description: "Connect to bitcoind", |
There was a problem hiding this comment.
Also uses a bitcoind binary? Maybe a comment if so, quite surprising and confusing. I also noticed above that walletTypeRPC = "bitcoindRPC".
| netPorts = dexbtc.NetPorts{ | ||
| Mainnet: "8332", | ||
| Testnet: "18332", | ||
| Simnet: "18443", | ||
| } |
There was a problem hiding this comment.
Not a biggie, but doesn't this increase the scope and lifetime of this variable, compared to when it was just local to a function?
| @@ -2092,8 +2313,8 @@ func (btc *ExchangeWallet) Address() (string, error) { | |||
|
|
|||
| // PayFee sends the dex registration fee. Transaction fees are in addition to | |||
| // the registration fee, and the fee rate is taken from the DEX configuration. | |||
| func (btc *ExchangeWallet) PayFee(address string, regFee uint64) (asset.Coin, error) { | |||
| txHash, vout, sent, err := btc.send(address, regFee, btc.feeRateWithFallback(1, 0), false) | |||
| func (btc *ExchangeWallet) PayFee(address string, regFee, feeRateSuggestion uint64) (asset.Coin, error) { | |||
There was a problem hiding this comment.
We still need similar changes for refund redeem and withdraw I think.
Just need to be careful not to make server RPCs to fetch the rate on paths that block other actions.
|
Looks like our balance check is a race with the wallet's internal scan after a block notification. It took two blocks to see a balance when it should have been one. Damn. |
Add a built-in Bitcoin SPV wallet. Uses neutrino and btcwallet.
client/asset:
Add Create and Exists methods to the Driver. Create is used to
create a "seeded" (native, built-in) wallet, and should
not be used for non-seeded wallets. Exists checks the existence of
a wallet. Exists is written so far to check even RPC wallet
existence, though it's only used for seeded wallets in production.
Adds the WalletDefinition type. WalletInfo has a new field,
AvailableWallets []*WalletDefinition.
WalletConfig type has a new field, Type string, which should match
the Type field of one of the AvailableWallets. Existing backends
must handle Type == "" for pre-existing wallets.
client/asset/{dcr,ltc,bch,eth}: minor adaptations to fit the
Driver and WalletDefinition changes.
client/asset/btc:
Quite a few changes to configuration parsing to more clearly
separate RPC config from wallet settings. New spvWallet type
manages the *btcwallet.Wallet and *neutrino.ChainService. The
Wallet interface was already defined, so the work was just
implementing the methods using direct calls to wallet methods.
Everything is run through an interface for testing. Existing tests
required relatively few modifications to work with spv as well.
There are just a couple of new unit tests specific to the more
nuanced spv methods. The regnet_test.go/livetest.go test is adapted
to run an all-SPV trio for one test.
client/core: Work with wallet definition and give special handling
to seeded wallets.
front-end:
NewWalletForm and WalletConfigForm updated to work with
wallet definitions. Wallet type is selected via a tab at the top of
the NewWalletForm. Seeded wallets take no user-supplied password
(enforced in core). Special handling of headers and options so that
creating a seeded wallet can be very close to a 1-button, 1-click
experience.
A new form is added after the new wallet form conditionally if the
wallet balance is lower than required for the fee. This will always
be the case for the first native spv wallet. The new form shows a
deposit address and the current balance, and directs the user to
fund the address for registration fees. The balance is checked in
a loop and progresses back to the fee confirmation form when the
balance is sufficient. When working on simnet, just fund it from
the alpha node in harness-ctl.
client/db: Store the new Type field as part of the wallet. I wrote
an upgrader to add the new field, but we're not populating it
anyway, and existing wallets handle that, so I don't see a good
reason to "upgrade".
Refactor initialization and connection. Move nearly all instantiation to connect. There is an assumption here that the caller will never try to use an unconnected wallet to do anything. Upon inspection, I think this is how we've designed things, but it's worth mentioning since calling some methods before connecting will definitely panic now. Log btcwallet and neutrino output to a rotating log file. Combine loadChainClient and startWallet functions into a single (*spvWallet).startWallet method.
Unlike bitcoind, btcwallet has option to subtract fees from sent value. Implement send with subtract from scratch.
|
We still have a lag in reporting correct balance, but other issues should be addressed now. |
| oldDef, err := walletDefinition(assetID, oldWallet.walletType) | ||
| // Error can be normal if the wallet was created before wallet types | ||
| // were a thing. Just assume this is an old wallet and therefore not | ||
| // seeded. | ||
| if err == nil && oldDef.Seeded { |
There was a problem hiding this comment.
You already have oldDef from 30 lines up before if walletDef.Seeded.
It also looks like you solved the problem in the comment with LegacyWalletIndex
There was a problem hiding this comment.
Should be good without the walletDefinition call and using the oldDef you have like:
if oldDef.Seeded && oldWallet.connected() {
oldWallet.Disconnect()
restartOnFail = true
}There was a problem hiding this comment.
NVM, I have some updates that will follow. Going to merge this shortly.
Add a built-in Bitcoin SPV wallet. Uses neutrino and btcwallet.
client/asset:
Add
CreateandExistsmethods to theDriver.Createis used tocreate a "seeded" (native, built-in) wallet, and should
not be used for non-seeded wallets.
Existschecks the existence ofa wallet.
Existsis written so far to check even RPC walletexistence, though it's only used for seeded wallets right now.
Adds the
WalletDefinitiontype.WalletInfohas a new field,AvailableWallets []*WalletDefinition.WalletConfigtype has a new field,Type string, which should matchthe
Typefield of one of theAvailableWallets. Existing backendsmust handle
Type == ""for pre-existing wallets.client/asset/btc:
Quite a few changes to configuration parsing to more clearly
separate RPC config from wallet settings. New
spvWallettypemanages the
*btcwallet.Walletand*neutrino.ChainService. TheWalletinterface was already defined, so the work was justimplementing the methods using direct calls to wallet methods.
Everything is run through an interface for testing. Existing tests
required relatively few modifications to work with spv as well.
There are just a couple of new unit tests specific to the more
nuanced spv methods. The regnet_test.go/livetest.go test is adapted
to run an all-SPV trio for one test.
client/core: Work with new wallet definitions and give special handling
to seeded wallets.
front-end:
NewWalletFormandWalletConfigFormupdated to work withwallet definitions. Wallet type is selected via a tab at the top of
the
NewWalletForm. Seeded wallets take no user-supplied password(enforced in core). Special handling of headers and options so that
creating a seeded wallet can be very close to a 1-button, 1-click
experience.
A new form is added after the new wallet form conditionally if the
wallet balance is lower than required for the fee. This will always
be the case for the first native spv wallet. The new form shows a
deposit address and the current balance, and directs the user to
fund the address for registration fees. The balance is checked in
a loop and progresses back to the fee confirmation form when the
balance is sufficient. When working on simnet, just fund it from
the alpha node in harness-ctl.
client/db:
Store the new
Typefield as part of the wallet. I wrotean upgrade function to store an empty string for the new fields
position in the database, but we're not populating it anyway,
and existing wallets handle that, so I don't see a good reason
to "upgrade".