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.

Sign up for GitHub

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

Implement external signer for dataposter #1919

Merged
merged 16 commits into from
Oct 17, 2023
Merged

Implement external signer for dataposter #1919

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
a619d43
Implement external signer for dataposter
anodar Oct 13, 2023
62801eb
Merge branch 'master' into external-signer
anodar Oct 13, 2023
4269470
merge master
anodar Oct 13, 2023
c3ed563
Drop accidentally added server binary for manual testing
anodar Oct 13, 2023
c177f02
Fix linter errors
anodar Oct 13, 2023
fa2f6fa
Merge branch 'master' into external-signer
anodar Oct 13, 2023
2f7b867
Make signing method configurable to allow both eth_signTransaction an…
anodar Oct 13, 2023
cfb4bc7
Merge branch 'external-signer' of github.com:OffchainLabs/nitro into …
anodar Oct 13, 2023
c27cb4b
Update arbnode/dataposter/data_poster.go
anodar Oct 16, 2023
73eb315
Merge branch 'master' into external-signer
anodar Oct 16, 2023
4661888
Check that signed transaction is the one sent for signing
anodar Oct 16, 2023
3816204
Merge branch 'external-signer' of github.com:OffchainLabs/nitro into …
anodar Oct 16, 2023
c551ff5
List transactions in correct order in the log line
anodar Oct 16, 2023
412a7e0
Merge branch 'master' into external-signer
anodar Oct 17, 2023
34fe2d0
Add default method for external signer
anodar Oct 17, 2023
1e9b588
Merge branch 'external-signer' of github.com:OffchainLabs/nitro into …
anodar Oct 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
138 changes: 106 additions & 32 deletions arbnode/dataposter/data_poster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,12 @@ import (

"github.com/ethereum/go-ethereum/accounts/abi/bind"
"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/common/hexutil"
"github.com/ethereum/go-ethereum/core/types"
"github.com/ethereum/go-ethereum/ethdb"
"github.com/ethereum/go-ethereum/log"
"github.com/ethereum/go-ethereum/params"
"github.com/ethereum/go-ethereum/rlp"
"github.com/ethereum/go-ethereum/rpc"
"github.com/go-redis/redis/v8"
"github.com/offchainlabs/nitro/arbnode/dataposter/dbstorage"
Expand Down Expand Up @@ -47,7 +49,7 @@ type DataPoster struct {
headerReader *headerreader.HeaderReader
client arbutil.L1Interface
sender common.Address
signer bind.SignerFn
signer signerFn
redisLock AttemptLocker
config ConfigFetcher
replacementTimes []time.Duration
Expand All @@ -66,6 +68,11 @@ type DataPoster struct {
errorCount map[uint64]int // number of consecutive intermittent errors rbf-ing or sending, per nonce
}

// signerFn is a signer function callback when a contract requires a method to
// sign the transaction before submission.
// This can be local or external, hence the context parameter.
type signerFn func(context.Context, common.Address, *types.Transaction) (*types.Transaction, error)

type AttemptLocker interface {
AttemptLock(context.Context) bool
}
Expand All @@ -85,7 +92,7 @@ func parseReplacementTimes(val string) ([]time.Duration, error) {
lastReplacementTime = t
}
if len(res) == 0 {
log.Warn("disabling replace-by-fee for data poster")
log.Warn("Disabling replace-by-fee for data poster")
}
// To avoid special casing "don't replace again", replace in 10 years.
return append(res, time.Hour*24*365*10), nil
Expand All @@ -103,13 +110,13 @@ type DataPosterOpts struct {
}

func NewDataPoster(ctx context.Context, opts *DataPosterOpts) (*DataPoster, error) {
initConfig := opts.Config()
replacementTimes, err := parseReplacementTimes(initConfig.ReplacementTimes)
cfg := opts.Config()
replacementTimes, err := parseReplacementTimes(cfg.ReplacementTimes)
if err != nil {
return nil, err
}
if opts.HeaderReader.IsParentChainArbitrum() && !initConfig.UseNoOpStorage {
initConfig.UseNoOpStorage = true
if opts.HeaderReader.IsParentChainArbitrum() && !cfg.UseNoOpStorage {
cfg.UseNoOpStorage = true
log.Info("Disabling data poster storage, as parent chain appears to be an Arbitrum chain without a mempool")
}
encF := func() storage.EncoderDecoderInterface {
Expand All @@ -120,17 +127,17 @@ func NewDataPoster(ctx context.Context, opts *DataPosterOpts) (*DataPoster, erro
}
var queue QueueStorage
switch {
case initConfig.UseNoOpStorage:
case cfg.UseNoOpStorage:
queue = &noop.Storage{}
case opts.RedisClient != nil:
var err error
queue, err = redisstorage.NewStorage(opts.RedisClient, opts.RedisKey, &initConfig.RedisSigner, encF)
queue, err = redisstorage.NewStorage(opts.RedisClient, opts.RedisKey, &cfg.RedisSigner, encF)
if err != nil {
return nil, err
}
case initConfig.UseDBStorage:
case cfg.UseDBStorage:
storage := dbstorage.New(opts.Database, func() storage.EncoderDecoderInterface { return &storage.EncoderDecoder{} })
if initConfig.Dangerous.ClearDBStorage {
if cfg.Dangerous.ClearDBStorage {
if err := storage.PruneAll(ctx); err != nil {
return nil, err
}
Expand All @@ -139,18 +146,64 @@ func NewDataPoster(ctx context.Context, opts *DataPosterOpts) (*DataPoster, erro
default:
queue = slice.NewStorage(func() storage.EncoderDecoderInterface { return &storage.EncoderDecoder{} })
}
return &DataPoster{
headerReader: opts.HeaderReader,
client: opts.HeaderReader.Client(),
sender: opts.Auth.From,
signer: opts.Auth.Signer,
dp := &DataPoster{
headerReader: opts.HeaderReader,
client: opts.HeaderReader.Client(),
sender: opts.Auth.From,
signer: func(_ context.Context, addr common.Address, tx *types.Transaction) (*types.Transaction, error) {
return opts.Auth.Signer(addr, tx)
},
config: opts.Config,
replacementTimes: replacementTimes,
metadataRetriever: opts.MetadataRetriever,
queue: queue,
redisLock: opts.RedisLock,
errorCount: make(map[uint64]int),
}, nil
}
if cfg.ExternalSigner.URL != "" {
signer, sender, err := externalSigner(ctx, &cfg.ExternalSigner)
if err != nil {
return nil, err
}
dp.signer, dp.sender = signer, sender
}
return dp, nil
}

// externalSigner returns signer function and ethereum address of the signer.
// Returns an error if address isn't specified or if it can't connect to the
// signer RPC server.
func externalSigner(ctx context.Context, opts *ExternalSignerCfg) (signerFn, common.Address, error) {
if opts.Address == "" {
return nil, common.Address{}, errors.New("external signer (From) address specified")
}
sender := common.HexToAddress(opts.Address)
client, err := rpc.DialContext(ctx, opts.URL)
if err != nil {
return nil, common.Address{}, fmt.Errorf("error connecting external signer: %w", err)
}

var hasher types.Signer
return func(ctx context.Context, addr common.Address, tx *types.Transaction) (*types.Transaction, error) {
// According to the "eth_signTransaction" API definition, this should be
// RLP encoded transaction object.
// https://ethereum.org/en/developers/docs/apis/json-rpc/#eth_signtransaction
var data hexutil.Bytes
if err := client.CallContext(ctx, &data, opts.Method, tx); err != nil {
return nil, fmt.Errorf("signing transaction: %w", err)
}
var signedTx types.Transaction
if err := rlp.DecodeBytes(data, &signedTx); err != nil {
return nil, fmt.Errorf("error decoding signed transaction: %w", err)
}
if hasher == nil {
hasher = types.LatestSignerForChainID(tx.ChainId())
}
if hasher.Hash(tx) != hasher.Hash(&signedTx) {
return nil, fmt.Errorf("transaction: %x from external signer differs from request: %x", hasher.Hash(&signedTx), hasher.Hash(tx))
}
return &signedTx, nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to add a safety check that this is the same transaction we requested to be signed. I think there's potential security issues without that, where a remote signer MITM returns a different tx with a bad signature, and then convinces the data poster to request that different tx be signed against the real remote signer.

The ideal way to do this would be to get a geth types.Signer (really confusingly named, it's more of a signature verifier than a signer, it doesn't have a private key or anything), and use its Hash method which returns the pre-signature hash, to verify that aside from the signature the txs are the same. I think we can get a "signer" by calling types.LatestSignerForChainID with the parent chain chain id from the config (we'll probably add that as a constructor argument to the data poster).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Added the check, although doing the lazy initialization, otherwise passing the signer (or chain ID) all the way from nitro.go would unnecessarily pollute call chain.

}, sender, nil
}

func (p *DataPoster) Sender() common.Address {
Expand Down Expand Up @@ -371,7 +424,7 @@ func (p *DataPoster) PostTransaction(ctx context.Context, dataCreatedAt time.Tim
Data: calldata,
AccessList: accessList,
}
fullTx, err := p.signer(p.sender, types.NewTx(&inner))
fullTx, err := p.signer(ctx, p.sender, types.NewTx(&inner))
if err != nil {
return nil, fmt.Errorf("signing transaction: %w", err)
}
Expand Down Expand Up @@ -450,7 +503,7 @@ func (p *DataPoster) replaceTx(ctx context.Context, prevTx *storage.QueuedTransa
newTx.Sent = false
newTx.Data.GasFeeCap = newFeeCap
newTx.Data.GasTipCap = newTipCap
newTx.FullTx, err = p.signer(p.sender, types.NewTx(&newTx.Data))
newTx.FullTx, err = p.signer(ctx, p.sender, types.NewTx(&newTx.Data))
if err != nil {
return err
}
Expand Down Expand Up @@ -636,20 +689,32 @@ type DataPosterConfig struct {
ReplacementTimes string `koanf:"replacement-times"`
// This is forcibly disabled if the parent chain is an Arbitrum chain,
// so you should probably use DataPoster's waitForL1Finality method instead of reading this field directly.
WaitForL1Finality bool `koanf:"wait-for-l1-finality" reload:"hot"`
MaxMempoolTransactions uint64 `koanf:"max-mempool-transactions" reload:"hot"`
MaxQueuedTransactions int `koanf:"max-queued-transactions" reload:"hot"`
TargetPriceGwei float64 `koanf:"target-price-gwei" reload:"hot"`
UrgencyGwei float64 `koanf:"urgency-gwei" reload:"hot"`
MinFeeCapGwei float64 `koanf:"min-fee-cap-gwei" reload:"hot"`
MinTipCapGwei float64 `koanf:"min-tip-cap-gwei" reload:"hot"`
MaxTipCapGwei float64 `koanf:"max-tip-cap-gwei" reload:"hot"`
NonceRbfSoftConfs uint64 `koanf:"nonce-rbf-soft-confs" reload:"hot"`
AllocateMempoolBalance bool `koanf:"allocate-mempool-balance" reload:"hot"`
UseDBStorage bool `koanf:"use-db-storage"`
UseNoOpStorage bool `koanf:"use-noop-storage"`
LegacyStorageEncoding bool `koanf:"legacy-storage-encoding" reload:"hot"`
Dangerous DangerousConfig `koanf:"dangerous"`
WaitForL1Finality bool `koanf:"wait-for-l1-finality" reload:"hot"`
MaxMempoolTransactions uint64 `koanf:"max-mempool-transactions" reload:"hot"`
MaxQueuedTransactions int `koanf:"max-queued-transactions" reload:"hot"`
TargetPriceGwei float64 `koanf:"target-price-gwei" reload:"hot"`
UrgencyGwei float64 `koanf:"urgency-gwei" reload:"hot"`
MinFeeCapGwei float64 `koanf:"min-fee-cap-gwei" reload:"hot"`
MinTipCapGwei float64 `koanf:"min-tip-cap-gwei" reload:"hot"`
MaxTipCapGwei float64 `koanf:"max-tip-cap-gwei" reload:"hot"`
NonceRbfSoftConfs uint64 `koanf:"nonce-rbf-soft-confs" reload:"hot"`
AllocateMempoolBalance bool `koanf:"allocate-mempool-balance" reload:"hot"`
UseDBStorage bool `koanf:"use-db-storage"`
UseNoOpStorage bool `koanf:"use-noop-storage"`
LegacyStorageEncoding bool `koanf:"legacy-storage-encoding" reload:"hot"`
Dangerous DangerousConfig `koanf:"dangerous"`
ExternalSigner ExternalSignerCfg `koanf:"external-signer"`
}

type ExternalSignerCfg struct {
// URL of the external signer rpc server, if set this overrides transaction
// options and uses external signer
// for signing transactions.
URL string `koanf:"url"`
// Hex encoded ethereum address of the external signer.
Address string `koanf:"address"`
// API method name (e.g. eth_signTransaction).
Method string `koanf:"method"`
}

type DangerousConfig struct {
Expand Down Expand Up @@ -680,12 +745,19 @@ func DataPosterConfigAddOptions(prefix string, f *pflag.FlagSet, defaultDataPost

signature.SimpleHmacConfigAddOptions(prefix+".redis-signer", f)
addDangerousOptions(prefix+".dangerous", f)
addExternalSignerOptions(prefix+".external-signer", f)
}

func addDangerousOptions(prefix string, f *pflag.FlagSet) {
f.Bool(prefix+".clear-dbstorage", DefaultDataPosterConfig.Dangerous.ClearDBStorage, "clear database storage")
}

func addExternalSignerOptions(prefix string, f *pflag.FlagSet) {
f.String(prefix+".url", DefaultDataPosterConfig.ExternalSigner.URL, "external signer url")
f.String(prefix+".address", DefaultDataPosterConfig.ExternalSigner.Address, "external signer address")
f.String(prefix+".method", DefaultDataPosterConfig.ExternalSigner.Method, "external signer method")
}

var DefaultDataPosterConfig = DataPosterConfig{
ReplacementTimes: "5m,10m,20m,30m,1h,2h,4h,6h,8h,12h,16h,18h,20h,22h",
WaitForL1Finality: true,
Expand All @@ -700,6 +772,7 @@ var DefaultDataPosterConfig = DataPosterConfig{
UseNoOpStorage: false,
LegacyStorageEncoding: true,
Dangerous: DangerousConfig{ClearDBStorage: false},
ExternalSigner: ExternalSignerCfg{},
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should still keep the default method as eth_signTransaction as that seems to be the most common method name (also applies to DefaultDataPosterConfigForValidator)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. ( I guess you meant TestDataPosterConfig not DefaultDataPosterConfigForValidator)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I got confused by the github diff

image

}

var DefaultDataPosterConfigForValidator = func() DataPosterConfig {
Expand All @@ -721,6 +794,7 @@ var TestDataPosterConfig = DataPosterConfig{
AllocateMempoolBalance: true,
UseDBStorage: false,
UseNoOpStorage: false,
ExternalSigner: ExternalSignerCfg{},
}

var TestDataPosterConfigForValidator = func() DataPosterConfig {
Expand Down
Loading
Loading