Skip to content
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

refactor: move towards more configurable implementation details #5708

Merged
merged 16 commits into from
Oct 8, 2021

Conversation

guggero
Copy link
Collaborator

@guggero guggero commented Sep 10, 2021

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
Copy link

orijbot commented Sep 10, 2021

lnd.go Show resolved Hide resolved
chainreg/chainregistry.go Outdated Show resolved Hide resolved
if err != nil {
return err
}
for method, ops := range r.implCfg.ExternalValidator.Permissions() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer required to be conditional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

config_builder.go Show resolved Hide resolved

// 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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@guggero guggero force-pushed the remote-signer-extract-wallet branch 3 times, most recently from bd4370e to 0599171 Compare September 13, 2021 14:59
@Roasbeef Roasbeef added this to the v0.14.0 milestone Sep 17, 2021
@Roasbeef 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 guggero force-pushed the remote-signer-extract-wallet branch 2 times, most recently from b3cedcd to af86883 Compare September 23, 2021 14:54
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🌃

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

privKey, err := b.DerivePrivKey(keyDesc)
privKey, err := b.DerivePrivKey(KeyDescriptor{
KeyLocator: keyLoc,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator Author

guggero commented Sep 24, 2021

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
Copy link
Collaborator Author

guggero commented Sep 27, 2021

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

@carlaKC carlaKC self-requested a review September 28, 2021 06:49
Copy link
Collaborator

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌮

// 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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

config.go Outdated
@@ -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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"when creating the chain control" is this correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

guggero commented Sep 29, 2021

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

@Roasbeef
Copy link
Member

Needs a rebase.

@guggero
Copy link
Collaborator Author

guggero commented Sep 30, 2021

Rebased.

@Roasbeef
Copy link
Member

Roasbeef commented Oct 1, 2021

Good thing we have such good itest coverage!

itests FTW 😎

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

guggero commented Oct 4, 2021

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

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

Roasbeef commented Oct 5, 2021

#5811 has been merged

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

guggero commented Oct 7, 2021

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

@Roasbeef
Copy link
Member

Roasbeef commented Oct 7, 2021

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 guggero force-pushed the remote-signer-extract-wallet branch from 3432cc9 to 0cfe0f4 Compare October 8, 2021 10:22
@guggero
Copy link
Collaborator Author

guggero commented Oct 8, 2021

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 guggero merged commit 00af978 into lightningnetwork:master Oct 8, 2021
@guggero guggero deleted the remote-signer-extract-wallet branch October 8, 2021 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accounting code health Related to code commenting, refactoring, and other non-behaviour improvements config Parameters/arguments/config file related issues/PRs refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants