implement live wallet reconfiguration #1686

buck54321 · 2022-06-25T02:26:13Z

Starts resolving #1492 . Just sketching this out for now. I can add other wallets here, or that can be followup work. The btc changes are in anticipation of changes coming in #1657.

Our current scheme for configuration changes requires killing the
existing wallet and creating a new one. Unfortunately, even though
we allow reconfiguration with active matches/orders, we've made
no accommodations for re-establishing locked coins on the new wallet.

client/asset:
Introduce the LiveReconfigurer interface with a single
method, Reconfigure(WalletConfig) (restartRequired bool, err error).
If a wallet implements LiverReconfigurer,

client/core:
Core will call Reconfigure early in (*Core).ReconfigureWallet, and
if there is no error and restartRequired is false, then the
reconfiguration is considered complete and no new wallet is created.
If, on the other hand, the LiveReconfigurer indicates that a restart
is required, we use our existing ReconfigureWallet code without
modification, but add a step at the end to re-reserve funding coin
and refund/redeem reserves. This new method (*Core).reReserveFunding
is modeled closely after resumeTrades, but optimized for only 1 asset
an no audits.

client/asset/btc:
Implement Reconfigure. Puts user-configurable baseWallet configuration
in a seperate struct, stored as an atomic.Value baseWallet field, with
the settings themselves readable through new getters. rpcWallet does
something similar with its an atomic.Value for its requester
RawRequesterWithContext field. The only thing that requires a restart
now is a wallet type change, though an error is generated if there is
an attempt to reduce the spv wallet birthday with special:activelyUsed set.

chappjc · 2022-07-04T22:52:54Z

Looks like you're building on #1657, in spirit. I'm fine with that one so wanna merge that so this can get rebased?

dex/config/config_test.go

buck54321 · 2022-07-05T01:06:59Z

I'm opening this up for review. I can add other assets here, but following up is good too.

martonp

Works well. I just see two minor UI issues:

If I try to switch wallet types while I have an active order, it fails with an error message as it should, but on the UI the wallet is shown as locked and unsynced. This fixes itself after refreshing the screen.
If I'm trying to switch from the SPV wallet to the external wallet while I have a trade running, it fails as it should, but if I navigate away to the markets page, then come back to the reconfigure screen, the RPC wallet settings are still showing even though I'm not using an SPV wallet. When I'm using the RPC wallet and I fail at switching to the SPV wallet, then I navigate away and come back to the reconfigure screen, the RPC wallet configs are still showing as they are supposed to. The issue only happens when attempting to switch from SPV to RPC.

martonp · 2022-07-07T13:21:46Z

client/asset/btc/rpcclient.go

+	// Check the RPC configuration.
+	newCfg := &parsedCfg.RPCConfig
+	if err := dexbtc.CheckRPCConfig(&newCfg.RPCConfig, wc.cloneParams.WalletInfo.Name, wc.cloneParams.Network, wc.cloneParams.Ports); err != nil {
+		return false, err


You can just do return here as you did earlier in the function.

JoeGruffins

Looks fine and tests well.

client/asset/btc/btc.go

JoeGruffins · 2022-07-13T03:22:05Z

client/asset/btc/spv.go

+	if err := w.updateDBBirthday(newBday); err != nil {
+		return false, fmt.Errorf("error storing new birthday")
+	}
+	w.birthdayV.Store(newBday)
+	if rescanRequired {
+		if err = w.rescanWalletAsync(); err != nil {
+			return false, fmt.Errorf("error initiating rescan after birthday adjustment")
+		}
+	}


Could include the error in these two.

JoeGruffins · 2022-07-13T04:39:16Z

client/core/core.go

+			c.log.Warnf("Error getting balance for wallet %s: %v", unbip(assetID), err)
+			// Do not fail in case this requires an unlocked wallet.


I guess this would also fail if the wallet was rescanning because the birthday was set lower?

JoeGruffins · 2022-07-13T05:03:32Z

client/core/core.go

+		return nil
+	}
+
+	clearTickGovernors := func() {


I can't figure out why it is necessary to clear these tick blocking timers. Why do we do this?

The tick governor meters attempts at swapping when there is repeating error. A new wallet might fix the issue, so we allow a tick to occur immediately.

Ok. I didn't realize this method is expected to be used if something was erroring. No problem.

JoeGruffins · 2022-07-13T05:12:57Z

client/core/core.go

+				fromID = tracker.Base()
+			}
+
+			denom, marketMult, limitMult := lcm(uint64(len(tracker.matches)), trade.Quantity)


@chappjc changes this up in #1653

That's fine. May not go that way for a while if things are OK for now. Can rebase it in the future.

However, there are a number of bug fixes and simplifications I listed that we should probably get eyes on and extract: #1653 (review)

go.mod

client/asset/btc/rpcclient.go

client/asset/btc/btc.go

client/core/wallet.go

chappjc

LGTM. @JoeGruffins please circle back on your review when convenient. Thx.

JoeGruffins · 2022-07-14T02:17:09Z

Switching wallet types, rpc to spv looks fine, but spv to rpc will say I have no peers and not let me trade until I restart dexc.

Our current scheme for configuration changes requires killing the existing wallet and creating a new one. Unfortunately, even though we allow reconfiguration with active matches/orders, we've made no accomodations for re-establishing locked coins on the new wallet. client/asset: Introduce the LiveReconfigurer interface with a single method, Reconfigure(WalletConfig) (restartRequired bool, err error). If a wallet implements LiverReconfigurer, client/core: Core will call Reconfigure early in (*Core).ReconfigureWallet, and if there is no error and restartRequired is false, then the reconfiguration is considered complete and no new wallet is created. If, on the other hand, the LiveReconfigurer indicates that a restart is required, we use our existing ReconfigureWallet code without modification, but add a step at the end to re-reserve funding coin and refund/redeem reserves. This new method (*Core).reReserveFunding is modeled closely after resumeTrades, but optimized for only 1 asset an no audits. client/asset/btc: Implement Reconfigure. Puts user-configurable baseWallet configuration in a seperate struct, stored as an atomic.Value baseWallet field, with the settings themselves readable through new getters. rpcWallet does something similar with its an atomic.Value for its requester RawRequesterWithContext field. The only thing that requires a restart now is a wallet type change, though an error is generated if there is an attempt to reduce the spv wallet birthday with special:activelyUsed set.

buck54321 mentioned this pull request Jun 28, 2022

client: fix native btc wallet creation with fee limit smaller than 1sat/B #1657

Merged

buck54321 force-pushed the clean-reconfig branch 3 times, most recently from c7d43ca to 61e7751 Compare July 4, 2022 22:06

chappjc reviewed Jul 4, 2022

View reviewed changes

dex/config/config_test.go Outdated Show resolved Hide resolved

buck54321 force-pushed the clean-reconfig branch from 7923890 to 9b7ac90 Compare July 5, 2022 01:02

buck54321 marked this pull request as ready for review July 5, 2022 01:07

martonp reviewed Jul 8, 2022

View reviewed changes

chappjc added this to the 0.5 milestone Jul 12, 2022

JoeGruffins reviewed Jul 13, 2022

View reviewed changes

buck54321 force-pushed the clean-reconfig branch from 9b7ac90 to b5d792f Compare July 13, 2022 23:05

chappjc reviewed Jul 13, 2022

View reviewed changes

go.mod Show resolved Hide resolved

chappjc reviewed Jul 14, 2022

View reviewed changes

buck54321 force-pushed the clean-reconfig branch from f8baeba to 814d34f Compare July 14, 2022 01:05

chappjc requested a review from JoeGruffins July 14, 2022 01:09

chappjc approved these changes Jul 14, 2022

View reviewed changes

JoeGruffins approved these changes Jul 15, 2022

View reviewed changes

buck54321 added 5 commits July 15, 2022 12:34

use ,extends

fcb7db6

supress broadcasts for new wallets before address verification

6e98df2

doc broadcasting. other chappjc review

aa8e1f3

log peer count before blocking message

edeb1c9

buck54321 force-pushed the clean-reconfig branch from 3044c39 to edeb1c9 Compare July 15, 2022 17:38

chappjc merged commit 731081b into decred:master Jul 15, 2022

chappjc mentioned this pull request Jul 19, 2022

client: all state, including locked funds, are lost upon reconfiguring wallet. #1492

Closed

buck54321 mentioned this pull request Jul 22, 2022

Implement LiveReconfigurer on dcr and eth #1727

Closed

		c.log.Warnf("Error getting balance for wallet %s: %v", unbip(assetID), err)
		// Do not fail in case this requires an unlocked wallet.

implement live wallet reconfiguration #1686

implement live wallet reconfiguration #1686

Uh oh!

Conversation

buck54321 commented Jun 25, 2022

Uh oh!

chappjc commented Jul 4, 2022

Uh oh!

Uh oh!

buck54321 commented Jul 5, 2022

Uh oh!

martonp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoeGruffins left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chappjc left a comment

Choose a reason for hiding this comment

Uh oh!

JoeGruffins commented Jul 14, 2022

Uh oh!

Uh oh!