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

Feature external signer #6780

Merged
merged 38 commits into from
May 29, 2024
Merged

Feature external signer #6780

merged 38 commits into from
May 29, 2024

Conversation

ak88
Copy link
Contributor

@ak88 ak88 commented Feb 26, 2024

Adds clef as a remote signer.
https://geth.ethereum.org/docs/tools/clef/introduction

Changes

  • new ClefSigner that can communicate using JsonRpc
  • CliqueSealer will seal with remote signer if available
  • new flag --Mining.Signer for setting url of the remote signer
  • using flag KeyStore.BlockAuthorAccount will attempt to sign with that account if available on remote

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Documentation

Configuration of clef and using the correct flags needs to be documented.
https://geth.ethereum.org/docs/tools/clef/clique-signing#prepping-clef

Requires documentation update

  • Yes
  • No

@brbrr brbrr marked this pull request as ready for review March 14, 2024 08:35
@brbrr brbrr requested a review from rubo as a code owner March 14, 2024 08:35
Copy link
Contributor

@smartprogrammer93 smartprogrammer93 left a comment

Choose a reason for hiding this comment

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

Wny not implement this as a plugin instead. does not feel like it should be part of the main client

@ak88
Copy link
Contributor Author

ak88 commented Mar 14, 2024

Wny not implement this as a plugin instead. does not feel like it should be part of the main client

Good idea - i will refactor it into a plugin instead.

@ak88 ak88 marked this pull request as draft March 14, 2024 12:40
ak88 added 3 commits March 25, 2024 13:37
commit ab6f5be
Author: ak88 <anders.holmbjerg@hotmail.com>
Date:   Mon Mar 25 13:36:30 2024 +0100

    unittest

commit ad2bf4c
Author: ak88 <anders.holmbjerg@hotmail.com>
Date:   Mon Mar 25 11:49:35 2024 +0100

    small fixes

commit 503db49
Author: ak88 <anders.holmbjerg@hotmail.com>
Date:   Thu Mar 21 15:17:54 2024 +0100

    plugin methods

commit 1034179
Author: ak88 <anders.holmbjerg@hotmail.com>
Date:   Thu Mar 21 12:57:53 2024 +0100

    clef signer as  plugin
src/Nethermind/Nethermind.Consensus/ISigner.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Consensus.Clique/CliqueSealer.cs Outdated Show resolved Hide resolved
@@ -70,4 +70,6 @@ public string ExtraData
return _blocksConfig;
}
}

public string Signer { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it is not possible, but could we make it an Uri? type?

Copy link
Contributor Author

@ak88 ak88 Apr 15, 2024

Choose a reason for hiding this comment

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

Not possible with the current JsonSerializer, but a custom one could be made.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should write one, is easy - here with JSON.NET: https://stackoverflow.com/a/8087049/1187056

@ak88 ak88 marked this pull request as ready for review April 15, 2024 14:31
Comment on lines +98 to +104
IHeaderSigner signer = Substitute.For<IHeaderSigner>();
signer.CanSignHeader.Returns(true);
signer.CanSign.Returns(true);
signer.Address.Returns(new Address("0x7ffc57839b00206d1ad20c69a1981b489f772031"));
signer.Sign(Arg.Any<BlockHeader>()).Returns(new Signature(new byte[65]));
CliqueSealer sut = new CliqueSealer(signer, new CliqueConfig(), _snapshotManager, LimboLogs.Instance);
Block block = Rlp.Decode<Block>(new Rlp(Bytes.FromHexString(blockRlp)));
Copy link
Member

Choose a reason for hiding this comment

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

Duplicated code

Comment on lines +72 to +75
BlockHeader clone = header.Clone();
clone.ExtraData = SnapshotManager.SliceExtraSealFromExtraData(clone.ExtraData);
clone.Hash = headerHash;
signature = headerSigner.Sign(clone);
Copy link
Member

Choose a reason for hiding this comment

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

Can we hide it somewhere in IHeaderSigner or with some extension method?

Comment on lines +24 to +29
IJsonRpcClient client = Substitute.For<IJsonRpcClient>();
client.Post<string[]>("account_list").Returns(Task.FromResult<string[]?>([TestItem.AddressA!.ToString()]));
Task<string?> postMethod = client.Post<string>("account_signData", "text/plain", Arg.Any<string>(), Keccak.Zero);
var returnValue = (new byte[65]).ToHexString();
postMethod.Returns(returnValue);
ClefSigner sut = await ClefSigner.Create(client);
Copy link
Member

Choose a reason for hiding this comment

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

Similarly a lot of duplicate in setup

@ak88 ak88 merged commit 4020ade into master May 29, 2024
68 checks passed
@ak88 ak88 deleted the feature/external-signer branch May 29, 2024 08:41
@anhnhgutech
Copy link
Contributor

@ak88 Hi, I see that the SetupExternalSigner only happens once when the node starts, right?
If it fails, it won't be retried. This means the Clef must be ready before the node starts. Why don't we SetupExternalSigner even when the Clef isn't ready?

src/Nethermind/Nethermind.ExternalSigner.Plugin/ClefSignerPlugin.cs

@LukaszRozmej
Copy link
Member

@anhnhgutech that is a good catch, would you care to try creating PR that handles reconnections?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants