refactor: move towards more configurable implementation details #5708

guggero · 2021-09-10T15:36:49Z

Extracted part of #5689, this is now a refactoring PR only.

Refactors multiple parts of the way lnd receives the main implementations for some of the core interfaces around chain control and wallets.

orijbot · 2021-09-10T15:36:51Z

Visit https://dashboard.github.orijtech.com?back=0&pr=5708&remote=true&repo=guggero%2Flnd to see benchmark details.

lnd.go

chainreg/chainregistry.go

Roasbeef · 2021-09-10T22:52:13Z

rpcserver.go

-			if err != nil {
-				return err
-			}
+	for method, ops := range r.implCfg.ExternalValidator.Permissions() {


No longer required to be conditional?

No, since we expect all interfaces to be non-nil. But the default implementation can just return a nil map of permissions which means we don't add any external validators here.

Roasbeef · 2021-09-10T22:53:58Z

config_builder.go

+	// NOTE/TODO: This currently _needs_ to be the same instance as the
+	// GraphDB above until the separation of the two databases is fully
+	// complete!
+	ChanStateDB *channeldb.DB


Is this already complete? Or are you referring to the interaction between the two when we need to fetch all the known addrs for a given peer for the SCB stuff?

No, for that we need to take apart the address fetch logic you mentioned but also the schema migrations that currently are operating on both channel state and graph database at the same time.

config_builder.go

Roasbeef · 2021-09-10T22:59:33Z

config_builder.go

@@ -60,6 +70,11 @@ type ImplementationCfg struct {
 	// ExternalValidator is a type that can provide external macaroon
 	// validation.
 	ExternalValidator
+
+	// DatabaseBuilder is a function that creates instances for and


Any value in pulling this out into a single method interface like you've done the above?

Nope, just laziness on my part 😅 Reworked it into a single method interface now.

config_builder.go

Roasbeef · 2021-09-10T23:08:42Z

cmd/lnd/main.go


 	// Call the "real" main in a nested manner so the defers will properly
 	// be executed in the case of a graceful shutdown.
 	if err = lnd.Main(
-		loadedConfig, lnd.ListenerCfg{}, shutdownInterceptor,
+		loadedConfig, lnd.ListenerCfg{}, implCfg, shutdownInterceptor,


How is it envisioned that a new caller swaps out some of these interfaces? By create a new wrapper struct that implements some of the newly defined builder methods, which then intercepts them (to take precedence) from the default implementations?

Or perhaps via the functional arg additions that would be passed in as additional arguments into ImplementationConfig? (similar to how things are outlined in that original brain storm issue)

Yes, those are both good examples. Or just implement the interfaces fully. For now we use the "embed and intercept" way in #5689. But can rework this again later if needed. Just having everything behind interfaces should make this flexible enough for now I think.

Roasbeef

LGTM 🌃

Roasbeef · 2021-09-23T23:08:21Z

keychain/btcwallet.go

 	msg []byte) (*btcec.Signature, error) {

-	privKey, err := b.DerivePrivKey(keyDesc)
+	privKey, err := b.DerivePrivKey(KeyDescriptor{
+		KeyLocator: keyLoc,


Doesn't this break an assumption in the pool client?

Fortunately not, since on the RPC level we've always only supported the key locator and not key descriptor for the SignMessage{Compact} calls.

guggero · 2021-09-24T13:40:56Z

Sigh, turns out we sometimes do double-sha256 hashing while other times we only use a single round of sha256 when signing messages. Had to refactor the SignMessage{Compact} methods again.
Good thing we have such good itest coverage!

guggero · 2021-09-27T15:58:08Z

Rebased and added release notes. Need volunteers for a second reviewer.

carlaKC

🌮

carlaKC · 2021-09-29T09:33:34Z

chainreg/chainregistry.go

+// PartialChainControl contains all the primary interfaces of the chain control
+// that can be purely constructed from the global configuration. No wallet
+// instance is required for constructing this partial state.
+type PartialChainControl struct {


(useless) nit: Not sure about PartialChainControl as a name, but also can't think of a better one (Common/Global..?)

Yeah, naming is hard... But I think partial isn't too bad of a name here since we split the initialization into two parts, so it's only complete after the second call.

carlaKC · 2021-09-29T09:37:30Z

config.go

@@ -1508,6 +1508,17 @@ func (c *Config) graphDatabaseDir() string {
 	)
 }

+// ImplementationConfig returns the configuration of what actual implementations
+// should be used when creating the chain control.


"when creating the chain control" is this correct?

No, that's not entirely correct in the latest version anymore. Fixed.

guggero · 2021-09-29T14:56:28Z

@Roasbeef rebased. Going to re-request review just to get your ACK on the updated signing/hashing logic.

Roasbeef · 2021-09-30T02:46:20Z

Needs a rebase.

guggero · 2021-09-30T06:35:44Z

Rebased.

Roasbeef · 2021-10-01T02:56:25Z

Good thing we have such good itest coverage!

itests FTW 😎

guggero · 2021-10-04T13:04:08Z

Currently blocked by clean itest run (probably needs #5811).

Roasbeef · 2021-10-05T23:21:33Z

#5811 has been merged

guggero · 2021-10-07T14:15:40Z

Rebased on top of #5833 to see if itest failures still occur.

Roasbeef · 2021-10-07T19:47:58Z

Merged #5833

To make it possible to use a remote signrpc server as a signer for our wallet, we need to change our main interface to sign the message instead of the message's digest. Otherwise we'd need to alter the signrpc.SignMessage RPC to accept a digest instead of only the message which has security implications.

To make it possible to use a remote lnrpc server as a signer for our wallet, we need to change our main interface to sign the message instead of the message's digest. Otherwise we'd need to alter the lnrpc.SignMessage RPC to accept a digest instead of only the message which has security implications.

To simplify the API surface of a remote signer even more, we refactor the SignMessage and SignMessageCompact calls to only accept a key locator as we always know what key we're using for signing anyway.

To simplify the message signing API even further, we refactor the lnwallet.MessageSigner interface to use a key locator instead of the public key to identify which key should be signed with.

The block cache size in the chainreg.Config previously wasn't used but instead the block cache was passed in as a separate parameter. We replace the cache size with the actual cache in the config to streamline things somewhat.

We'll refactor the wallet creation and unlock process in a following commit and want to make it possible to not need a direct reference to the macaroon service in our main function. Since we store it in the interceptor chain anyway (if we're using macaroons in the first place), we might as well use the instance there directly.

As a preparation for separating the pure config related chain control setup from the wallet related chain control, we store more information in the chain control instance that is required for the wallet initialization.

To remove one more direct dependency to a variable in our main function, we pass in the required parameter to the autopilot only instead of the whole chain configuration.

As a preparation for adding the wallet unlock params to the chain control, we first need to move them out of the main package.

As a preparation for extracting the wallet related initialization code, we first need to separate the purely configuration related chain control initialization from the wallet related code. We do that by splitting the chain control into a partial and full struct that is initialized in two parts. This also allows us to create the wallet configuration itself outside of the chain control package and we need to thread through fewer parameters through the chain control config.

As a preparation for making more and more implementation details configurable, we add a new ImplementationCfg struct that houses all the interfaces that can be defined externally.

We move some of the wallet related functions into the new file that houses the new customizable implementations for some of our interfaces. Since the next customizable interface will be the wallet, we move those wallet and chain backend related helper functions. NOTE: This is a pure code move.

With this commit we extract the wallet creation/unlocking and initialization completely out of the main function. This will allow us to use custom implementations in the future.

guggero · 2021-10-08T12:22:43Z

Going to merge this. The ARM itest seems to have reached its maximum allowed run time of 60 minutes with just two tranches running in parallel. I increased that to 3 and now it's green.
There were two other flakes in the itests, including one that caused the test to time out as well. I identified it (caused by the REST WebSocket test) and will address it in a separate PR.
The third flake is the "channel announcement is premature" error that leads to a channel not being seen by one of the nodes. This also pre-existed and needs to be addressed separately.

guggero requested a review from Roasbeef September 10, 2021 15:36

guggero mentioned this pull request Sep 10, 2021

Support remote signing over RPC #5689

Merged

Roasbeef reviewed Sep 10, 2021

View reviewed changes

guggero force-pushed the remote-signer-extract-wallet branch 3 times, most recently from bd4370e to 0599171 Compare September 13, 2021 14:59

Roasbeef added this to the v0.14.0 milestone Sep 17, 2021

Roasbeef added accounting code health Related to code commenting, refactoring, and other non-behaviour improvements config Parameters/arguments/config file related issues/PRs refactoring labels Sep 17, 2021

guggero force-pushed the remote-signer-extract-wallet branch 2 times, most recently from b3cedcd to af86883 Compare September 23, 2021 14:54

Roasbeef approved these changes Sep 23, 2021

View reviewed changes

guggero force-pushed the remote-signer-extract-wallet branch from af86883 to a224c04 Compare September 24, 2021 13:39

guggero force-pushed the remote-signer-extract-wallet branch from a224c04 to d8da64d Compare September 27, 2021 15:57

carlaKC self-requested a review September 28, 2021 06:49

carlaKC approved these changes Sep 29, 2021

View reviewed changes

guggero force-pushed the remote-signer-extract-wallet branch 2 times, most recently from 7250b64 to e3df70f Compare September 29, 2021 14:51

guggero requested a review from Roasbeef September 29, 2021 14:56

guggero force-pushed the remote-signer-extract-wallet branch from e3df70f to 519d8be Compare September 30, 2021 06:35

guggero force-pushed the remote-signer-extract-wallet branch 2 times, most recently from f329c68 to d0569da Compare October 4, 2021 13:02

guggero force-pushed the remote-signer-extract-wallet branch from d0569da to 36f5765 Compare October 5, 2021 10:07

guggero force-pushed the remote-signer-extract-wallet branch 2 times, most recently from dfea280 to 3432cc9 Compare October 7, 2021 14:15

guggero added 16 commits October 8, 2021 12:06

multi: use key locator only for signing messages

afa03f2

To simplify the API surface of a remote signer even more, we refactor the SignMessage and SignMessageCompact calls to only accept a key locator as we always know what key we're using for signing anyway.

multi: use key locator for lnwallet.MessageSigner

e79d59d

To simplify the message signing API even further, we refactor the lnwallet.MessageSigner interface to use a key locator instead of the public key to identify which key should be signed with.

chainreg+lnd: add block cache to chainreg config

d121541

The block cache size in the chainreg.Config previously wasn't used but instead the block cache was passed in as a separate parameter. We replace the cache size with the actual cache in the config to streamline things somewhat.

chainreg: add chain source and channel constraints

cab800e

As a preparation for separating the pure config related chain control setup from the wallet related chain control, we store more information in the chain control instance that is required for the wallet initialization.

lnd+pilot: use minHTLCIn instead of whole config

862f712

To remove one more direct dependency to a variable in our main function, we pass in the required parameter to the autopilot only instead of the whole chain configuration.

walletunlocker+lnd: move WalletUnlockParams

47f1b81

As a preparation for adding the wallet unlock params to the chain control, we first need to move them out of the main package.

multi: refactor external subserver config

0e279eb

As a preparation for making more and more implementation details configurable, we add a new ImplementationCfg struct that houses all the interfaces that can be defined externally.

multi: extract database initialization

f6d7e70

multi: extract wallet initialization

e1da1f8

With this commit we extract the wallet creation/unlocking and initialization completely out of the main function. This will allow us to use custom implementations in the future.

docs: add release notes

82ae284

Travis: bump ARM parallelity to 3

0cfe0f4

guggero force-pushed the remote-signer-extract-wallet branch from 3432cc9 to 0cfe0f4 Compare October 8, 2021 10:22

guggero merged commit 00af978 into lightningnetwork:master Oct 8, 2021

guggero deleted the remote-signer-extract-wallet branch October 8, 2021 12:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move towards more configurable implementation details #5708

refactor: move towards more configurable implementation details #5708

guggero commented Sep 10, 2021

orijbot commented Sep 10, 2021

Roasbeef Sep 10, 2021

guggero Sep 13, 2021

Roasbeef Sep 10, 2021

guggero Sep 13, 2021

Roasbeef Sep 10, 2021

guggero Sep 13, 2021

Roasbeef Sep 10, 2021

Roasbeef Sep 10, 2021

guggero Sep 13, 2021

Roasbeef left a comment

Roasbeef Sep 23, 2021

guggero Sep 24, 2021

guggero commented Sep 24, 2021

guggero commented Sep 27, 2021

carlaKC left a comment

carlaKC Sep 29, 2021

guggero Sep 29, 2021

carlaKC Sep 29, 2021

guggero Sep 29, 2021

guggero commented Sep 29, 2021

Roasbeef commented Sep 30, 2021

guggero commented Sep 30, 2021

Roasbeef commented Oct 1, 2021

guggero commented Oct 4, 2021

Roasbeef commented Oct 5, 2021

guggero commented Oct 7, 2021

Roasbeef commented Oct 7, 2021

guggero commented Oct 8, 2021

refactor: move towards more configurable implementation details #5708

refactor: move towards more configurable implementation details #5708

Conversation

guggero commented Sep 10, 2021

orijbot commented Sep 10, 2021

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

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

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guggero commented Sep 24, 2021

guggero commented Sep 27, 2021

carlaKC left a 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

Choose a reason for hiding this comment

guggero commented Sep 29, 2021

Roasbeef commented Sep 30, 2021

guggero commented Sep 30, 2021

Roasbeef commented Oct 1, 2021

guggero commented Oct 4, 2021

Roasbeef commented Oct 5, 2021

guggero commented Oct 7, 2021

Roasbeef commented Oct 7, 2021

guggero commented Oct 8, 2021