From a45dac53a0e6f48cec0c971b81bf3e580c765c05 Mon Sep 17 00:00:00 2001 From: Nodar Date: Thu, 17 Aug 2023 17:54:00 +0200 Subject: [PATCH 01/22] Introduce koanf linter that checks that field names match koanf tags --- .gitignore | 1 + .gitmodules | 3 + .golangci.yml | 7 +++ Makefile | 4 +- go.mod | 17 +++--- go.sum | 34 ++++++----- linter/golangci-lint | 1 + linter/koanf/koanf.go | 116 +++++++++++++++++++++++++++++++++++++ linter/koanf/koanf_test.go | 31 ++++++++++ 9 files changed, 189 insertions(+), 25 deletions(-) create mode 160000 linter/golangci-lint create mode 100644 linter/koanf/koanf.go create mode 100644 linter/koanf/koanf_test.go diff --git a/.gitignore b/.gitignore index f0eb5c2ec3..8937c1b5d1 100644 --- a/.gitignore +++ b/.gitignore @@ -21,3 +21,4 @@ yarn-error.log local/ testdata system_tests/test-data/* +linter/koanf/koanf.so \ No newline at end of file diff --git a/.gitmodules b/.gitmodules index 7c78791c78..11d9caa2aa 100644 --- a/.gitmodules +++ b/.gitmodules @@ -20,3 +20,6 @@ [submodule "nitro-testnode"] path = nitro-testnode url = https://github.com/OffchainLabs/nitro-testnode.git +[submodule "linter/golangci-lint"] + path = linter/golangci-lint + url = https://github.com/golangci/golangci-lint diff --git a/.golangci.yml b/.golangci.yml index e794cdb844..952a52a0c8 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -23,6 +23,7 @@ linters: - nilerr # ensure errors aren't mishandled - staticcheck # check for suspicious constructs - unused # check for unused constructs + - koanf linters-settings: errcheck: @@ -51,3 +52,9 @@ linters-settings: disable: - shadow - fieldalignment + + + custom: + koanf: + path: linter/koanf/koanf.so + description: Koanf configuration linter diff --git a/Makefile b/Makefile index 205025dfe9..71c051087a 100644 --- a/Makefile +++ b/Makefile @@ -304,7 +304,9 @@ contracts/test/prover/proofs/%.json: $(arbitrator_cases)/%.wasm $(arbitrator_pro # strategic rules to minimize dependency building .make/lint: $(DEP_PREDICATE) build-node-deps $(ORDER_ONLY_PREDICATE) .make - golangci-lint run --fix + make build -C linter/golangci-lint + go build -buildmode=plugin -o linter/koanf/koanf.so linter/koanf/koanf.go + ./linter/golangci-lint/golangci-lint run --fix yarn --cwd contracts solhint @touch $@ diff --git a/go.mod b/go.mod index 5adfd19388..526f0a819f 100644 --- a/go.mod +++ b/go.mod @@ -19,6 +19,7 @@ require ( github.com/codeclysm/extract/v3 v3.0.2 github.com/dgraph-io/badger/v3 v3.2103.2 github.com/ethereum/go-ethereum v1.10.26 + github.com/fatih/structtag v1.2.0 github.com/google/go-cmp v0.5.9 github.com/hashicorp/golang-lru/v2 v2.0.1 github.com/ipfs/go-cid v0.3.2 @@ -31,7 +32,8 @@ require ( github.com/multiformats/go-multihash v0.2.1 github.com/spf13/pflag v1.0.5 github.com/wealdtech/go-merkletree v1.0.0 - golang.org/x/term v0.6.0 + golang.org/x/term v0.11.0 + golang.org/x/tools v0.12.0 gopkg.in/natefinch/lumberjack.v2 v2.0.0 ) @@ -256,8 +258,7 @@ require ( go.uber.org/zap v1.24.0 // indirect go4.org v0.0.0-20200411211856-f5505b9728dd // indirect golang.org/x/exp v0.0.0-20230206171751-46f607a40771 // indirect - golang.org/x/mod v0.9.0 // indirect - golang.org/x/tools v0.7.0 // indirect + golang.org/x/mod v0.12.0 // indirect golang.org/x/xerrors v0.0.0-20220907171357-04be3eba64a2 // indirect google.golang.org/genproto v0.0.0-20211118181313-81c1377c94b1 // indirect google.golang.org/grpc v1.46.0 // indirect @@ -309,11 +310,11 @@ require ( github.com/tklauser/go-sysconf v0.3.5 // indirect github.com/tklauser/numcpus v0.2.2 // indirect github.com/tyler-smith/go-bip39 v1.1.0 // indirect - golang.org/x/crypto v0.6.0 - golang.org/x/net v0.8.0 // indirect - golang.org/x/sync v0.1.0 // indirect - golang.org/x/sys v0.6.0 - golang.org/x/text v0.8.0 // indirect + golang.org/x/crypto v0.12.0 + golang.org/x/net v0.14.0 // indirect + golang.org/x/sync v0.3.0 // indirect + golang.org/x/sys v0.11.0 + golang.org/x/text v0.12.0 // indirect golang.org/x/time v0.0.0-20220922220347-f3bd1da661af // indirect gopkg.in/natefinch/npipe.v2 v2.0.0-20160621034901-c1b8fa8bdcce // indirect ) diff --git a/go.sum b/go.sum index 58155db124..43240135c5 100644 --- a/go.sum +++ b/go.sum @@ -326,6 +326,8 @@ github.com/facebookgo/atomicfile v0.0.0-20151019160806-2de1f203e7d5/go.mod h1:Jp github.com/fasthttp-contrib/websocket v0.0.0-20160511215533-1f3b11f56072/go.mod h1:duJ4Jxv5lDcvg4QuQr0oowTf7dz4/CR8NtyCooz9HL8= github.com/fatih/color v1.7.0/go.mod h1:Zm6kSWBoL9eyXnKyktHP6abPY2pDugNf5KwzbycvMj4= github.com/fatih/structs v1.1.0/go.mod h1:9NiDSp5zOcgEDl+j00MP/WkGVPOlPRLejGD8Ga6PJ7M= +github.com/fatih/structtag v1.2.0 h1:/OdNE99OxoI/PqaW/SuSK9uxxT3f/tcSZgon/ssNSx4= +github.com/fatih/structtag v1.2.0/go.mod h1:mBJUNpUnHmRKrKlQQlmCrh5PuhftFbNv8Ys4/aAZl94= github.com/fjl/memsize v0.0.0-20190710130421-bcb5799ab5e5 h1:FtmdgXiUlNeRsoNMFlKLDt+S+6hbjVMEW6RGQ7aUf7c= github.com/fjl/memsize v0.0.0-20190710130421-bcb5799ab5e5/go.mod h1:VvhXpOYNQvB+uIk2RvXzuaQtkQJzzIx6lSBe1xv7hi0= github.com/flynn/go-shlex v0.0.0-20150515145356-3f9db97f8568/go.mod h1:xEzjJPgXI435gkrCt3MPfRiAkVrwSbHsst4LCFVfpJc= @@ -1737,8 +1739,8 @@ golang.org/x/crypto v0.0.0-20210322153248-0c34fe9e7dc2/go.mod h1:T9bdIzuCu7OtxOm golang.org/x/crypto v0.0.0-20210506145944-38f3c27a63bf/go.mod h1:P+XmwS30IXTQdn5tA2iutPOUgjI07+tq3H3K9MVA1s8= golang.org/x/crypto v0.0.0-20210920023735-84f357641f63/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc= golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc= -golang.org/x/crypto v0.6.0 h1:qfktjS5LUO+fFKeJXZ+ikTRijMmljikvG68fpMMruSc= -golang.org/x/crypto v0.6.0/go.mod h1:OFC/31mSvZgRz0V1QTNCzfAI1aIRzbiufJtkMIlEp58= +golang.org/x/crypto v0.12.0 h1:tFM/ta59kqch6LlvYnPa0yx5a83cL2nHflFhYKvv9Yk= +golang.org/x/crypto v0.12.0/go.mod h1:NF0Gs7EO5K4qLn+Ylc+fih8BSTeIjAP05siRnAh98yw= golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= golang.org/x/exp v0.0.0-20190306152737-a1d7652674e8/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= golang.org/x/exp v0.0.0-20190510132918-efd6b22b2522/go.mod h1:ZjyILWgesfNpC6sMxTJOJm9Kp84zZh5NQWvqDGG3Qr8= @@ -1774,8 +1776,8 @@ golang.org/x/mod v0.1.1-0.20191107180719-034126e5016b/go.mod h1:QqPTAvyqsEbceGzB golang.org/x/mod v0.2.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.4.2/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= -golang.org/x/mod v0.9.0 h1:KENHtAZL2y3NLMYZeHY9DW8HW8V+kQyJsY/V9JlKvCs= -golang.org/x/mod v0.9.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= +golang.org/x/mod v0.12.0 h1:rmsUpXtvNzj340zd98LZ4KntptpfRHwpFOHG188oHXc= +golang.org/x/mod v0.12.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= golang.org/x/net v0.0.0-20180406214816-61147c48b25b/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20180719180050-a680a1efc54d/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= @@ -1835,8 +1837,8 @@ golang.org/x/net v0.0.0-20210428140749-89ef3d95e781/go.mod h1:OJAsFXCWl8Ukc7SiCT golang.org/x/net v0.0.0-20210726213435-c6fcb2dbf985/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y= golang.org/x/net v0.0.0-20210917221730-978cfadd31cf/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y= golang.org/x/net v0.0.0-20211008194852-3b03d305991f/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y= -golang.org/x/net v0.8.0 h1:Zrh2ngAOFYneWTAIAPethzeaQLuHwhuBkuV6ZiRnUaQ= -golang.org/x/net v0.8.0/go.mod h1:QVkue5JL9kW//ek3r6jTKnTFis1tRmNAW2P1shuFdJc= +golang.org/x/net v0.14.0 h1:BONx9s002vGdD9umnlX1Po8vOZmrgH34qlHcD1MfK14= +golang.org/x/net v0.14.0/go.mod h1:PpSgVXXLK0OxS0F31C1/tv6XNguvCrnXIDrFMspZIUI= golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U= golang.org/x/oauth2 v0.0.0-20181017192945-9dcd33a902f4/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U= golang.org/x/oauth2 v0.0.0-20181203162652-d668ce993890/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U= @@ -1857,8 +1859,8 @@ golang.org/x/sync v0.0.0-20200625203802-6e8e738ad208/go.mod h1:RxMgew5VJxzue5/jJ golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20201207232520-09787c993a3a/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20210220032951-036812b2e83c/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= -golang.org/x/sync v0.1.0 h1:wsuoTGHzEhffawBOhz5CYhcrV4IdKZbEyZjBMuTp12o= -golang.org/x/sync v0.1.0/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= +golang.org/x/sync v0.3.0 h1:ftCYgMx6zT/asHUrPw8BLLscYtGznsLAnjq5RH9P66E= +golang.org/x/sync v0.3.0/go.mod h1:FU7BRWz2tNW+3quACPkgCx/L+uEAv1htQ0V83Z9Rj+Y= golang.org/x/sys v0.0.0-20180810173357-98c5dad5d1a0/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20180823144017-11551d06cbcc/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20180830151530-49385e6e1522/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= @@ -1951,12 +1953,12 @@ golang.org/x/sys v0.0.0-20220209214540-3681064d5158/go.mod h1:oPkhp1MJrh7nUepCBc golang.org/x/sys v0.0.0-20220704084225-05e143d24a9e/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220908164124-27713097b956/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.6.0 h1:MVltZSvRTcU2ljQOhs94SXPftV6DCNnZViHeQps87pQ= -golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.11.0 h1:eG7RXZHdqOJ1i+0lgLgCpSXAp6M3LYlAo6osgSi0xOM= +golang.org/x/sys v0.11.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/term v0.0.0-20201117132131-f5c789dd3221/go.mod h1:Nr5EML6q2oocZ2LXRh80K7BxOlk5/8JxuGnuhpl+muw= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= -golang.org/x/term v0.6.0 h1:clScbb1cHjoCkyRbWwBEUZ5H/tIFu5TAXIqaZD0Gcjw= -golang.org/x/term v0.6.0/go.mod h1:m6U89DPEgQRMq3DNkDClhWw02AUbt2daBVO4cn4Hv9U= +golang.org/x/term v0.11.0 h1:F9tnn/DA/Im8nCwm+fX+1/eBwi4qFjRT++MhtVC4ZX0= +golang.org/x/term v0.11.0/go.mod h1:zC9APTIj3jG3FdV/Ons+XE1riIZXG4aZ4GTHiPZJPIU= golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.1-0.20180807135948-17ff2d5776d2/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= @@ -1966,8 +1968,8 @@ golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.3.5/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.3.6/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.3.7/go.mod h1:u+2+/6zg+i71rQMx5EYifcz6MCKuco9NR6JIITiCfzQ= -golang.org/x/text v0.8.0 h1:57P1ETyNKtuIjB4SRd15iJxuhj8Gc416Y78H3qgMh68= -golang.org/x/text v0.8.0/go.mod h1:e1OnstbJyHTd6l/uOt8jFFHp6TRDWZR/bV3emEE/zU8= +golang.org/x/text v0.12.0 h1:k+n5B8goJNdU7hSvEtMUz3d1Q6D/XW4COJSJR6fN0mc= +golang.org/x/text v0.12.0/go.mod h1:TvPlkZtksWOMsz7fbANvkp4WM8x/WCo/om8BMLbz+aE= golang.org/x/time v0.0.0-20180412165947-fbb02b2291d2/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ= golang.org/x/time v0.0.0-20181108054448-85acf8d2951c/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ= golang.org/x/time v0.0.0-20190308202827-9d24e82272b4/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ= @@ -2035,8 +2037,8 @@ golang.org/x/tools v0.1.1/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk= golang.org/x/tools v0.1.3/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk= golang.org/x/tools v0.1.5/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk= golang.org/x/tools v0.1.6-0.20210726203631-07bc1bf47fb2/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk= -golang.org/x/tools v0.7.0 h1:W4OVu8VVOaIO0yzWMNdepAulS7YfoS3Zabrm8DOXXU4= -golang.org/x/tools v0.7.0/go.mod h1:4pg6aUX35JBAogB10C9AtvVL+qowtN4pT3CGSQex14s= +golang.org/x/tools v0.12.0 h1:YW6HUoUmYBpwSgyaGaZq1fHjrBjX1rlpZ54T6mu2kss= +golang.org/x/tools v0.12.0/go.mod h1:Sc0INKfu04TlqNoRA1hgpFZbhYXHPr4V5DzpSBTPqQM= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= diff --git a/linter/golangci-lint b/linter/golangci-lint new file mode 160000 index 0000000000..8000abaf0e --- /dev/null +++ b/linter/golangci-lint @@ -0,0 +1 @@ +Subproject commit 8000abaf0e6e28e8179864f0317349cecab47c05 diff --git a/linter/koanf/koanf.go b/linter/koanf/koanf.go new file mode 100644 index 0000000000..4aaddaa495 --- /dev/null +++ b/linter/koanf/koanf.go @@ -0,0 +1,116 @@ +package main + +import ( + "fmt" + "go/ast" + "go/token" + "reflect" + "strings" + "unicode" + + "github.com/fatih/structtag" + + "golang.org/x/tools/go/analysis" +) + +func New(conf any) ([]*analysis.Analyzer, error) { + return []*analysis.Analyzer{Analyzer}, nil +} + +var Analyzer = &analysis.Analyzer{ + Name: "koanfcheck", + Doc: "check for koanf misconfigurations", + Run: func(p *analysis.Pass) (interface{}, error) { return run(false, p) }, + ResultType: reflect.TypeOf(Result{}), +} + +var analyzerForTests = &analysis.Analyzer{ + Name: "testkoanfcheck", + Doc: "check for koanf misconfigurations (for tests)", + Run: func(p *analysis.Pass) (interface{}, error) { return run(true, p) }, + ResultType: reflect.TypeOf(Result{}), +} + +// koanfError indicates the position of an error in configuration. +type koanfError struct { + Pos token.Position + Message string +} + +// Result is returned from the checkStruct function, and holds all the +// configuration errors. +type Result struct { + Errors []koanfError +} + +func run(dryRun bool, pass *analysis.Pass) (interface{}, error) { + var ret Result + for _, f := range pass.Files { + ast.Inspect(f, func(node ast.Node) bool { + var res Result + switch v := node.(type) { + case *ast.StructType: + res = checkStruct(pass, v) + default: + } + for _, err := range res.Errors { + ret.Errors = append(ret.Errors, err) + if !dryRun { + pass.Report(analysis.Diagnostic{ + Pos: pass.Fset.File(f.Pos()).Pos(err.Pos.Offset), + Message: err.Message, + Category: "koanf", + }) + } + } + return true + }, + ) + } + return ret, nil +} + +func checkStruct(pass *analysis.Pass, s *ast.StructType) Result { + var res Result + for _, f := range s.Fields.List { + if f.Tag == nil { + continue + } + tags, err := structtag.Parse(strings.Trim((f.Tag.Value), "`")) + if err != nil { + continue + } + tag, err := tags.Get("koanf") + if err != nil { + continue + } + tagName := normalize(tag.Name) + fieldName := f.Names[0].Name + if !strings.EqualFold(tagName, fieldName) { + res.Errors = append(res.Errors, koanfError{ + Pos: pass.Fset.Position(f.Pos()), + Message: fmt.Sprintf("field name: %q doesn't match tag name: %q\n", fieldName, tagName), + }) + } + } + return res +} + +func normalize(s string) string { + ans := s[:1] + for i := 1; i < len(s); i++ { + c := rune(s[i]) + if !isAlphanumeric(c) { + continue + } + if !isAlphanumeric(rune(s[i-1])) && unicode.IsLower(c) { + c = unicode.ToUpper(c) + } + ans += string(c) + } + return ans +} + +func isAlphanumeric(c rune) bool { + return unicode.IsLetter(c) || unicode.IsDigit(c) +} diff --git a/linter/koanf/koanf_test.go b/linter/koanf/koanf_test.go new file mode 100644 index 0000000000..2e3e68b0f4 --- /dev/null +++ b/linter/koanf/koanf_test.go @@ -0,0 +1,31 @@ +package main + +import ( + "os" + "path/filepath" + "testing" + + "golang.org/x/tools/go/analysis/analysistest" +) + +func TestAll(t *testing.T) { + wd, err := os.Getwd() + if err != nil { + t.Fatalf("Failed to get wd: %s", err) + } + testdata := filepath.Join(filepath.Dir(wd), "testdata") + res := analysistest.Run(t, testdata, analyzerForTests, "a") + if cnt := countErrors(res); cnt != 1 { + t.Errorf("analysistest.Run() got %v errors, expected 1", cnt) + } +} + +func countErrors(errs []*analysistest.Result) int { + cnt := 0 + for _, e := range errs { + if r, ok := e.Result.(Result); ok { + cnt += len(r.Errors) + } + } + return cnt +} From 7547edb550f6bca2761d2b06d761e6cf201cb7bf Mon Sep 17 00:00:00 2001 From: Nodar Date: Thu, 17 Aug 2023 17:56:28 +0200 Subject: [PATCH 02/22] Add empty line at the end of gitignore --- .gitignore | 2 +- .golangci.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.gitignore b/.gitignore index 8937c1b5d1..02cc86192b 100644 --- a/.gitignore +++ b/.gitignore @@ -21,4 +21,4 @@ yarn-error.log local/ testdata system_tests/test-data/* -linter/koanf/koanf.so \ No newline at end of file +linter/koanf/koanf.so diff --git a/.golangci.yml b/.golangci.yml index 952a52a0c8..8f2fdda037 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -23,7 +23,7 @@ linters: - nilerr # ensure errors aren't mishandled - staticcheck # check for suspicious constructs - unused # check for unused constructs - - koanf + - koanf # check for koanf configurations linters-settings: errcheck: From eba8989a80857d89573223c385ea5f4dabbe66a8 Mon Sep 17 00:00:00 2001 From: Nodar Date: Thu, 17 Aug 2023 17:57:31 +0200 Subject: [PATCH 03/22] drop extra empty line in golangci.yml --- .golangci.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.golangci.yml b/.golangci.yml index 8f2fdda037..47f471b5a1 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -53,7 +53,6 @@ linters-settings: - shadow - fieldalignment - custom: koanf: path: linter/koanf/koanf.so From a5c667343a62d839baa5384141001bd3b182778f Mon Sep 17 00:00:00 2001 From: Nodar Date: Fri, 18 Aug 2023 15:11:13 +0200 Subject: [PATCH 04/22] Run custom linter as go binary instead of plugin for golangci-lint, drop golangci-lint submodule --- .gitignore | 1 - .gitmodules | 3 --- .golangci.yml | 6 ------ Makefile | 4 +--- linter/golangci-lint | 1 - linter/koanf/koanf.go | 6 +++++- 6 files changed, 6 insertions(+), 15 deletions(-) delete mode 160000 linter/golangci-lint diff --git a/.gitignore b/.gitignore index 02cc86192b..f0eb5c2ec3 100644 --- a/.gitignore +++ b/.gitignore @@ -21,4 +21,3 @@ yarn-error.log local/ testdata system_tests/test-data/* -linter/koanf/koanf.so diff --git a/.gitmodules b/.gitmodules index 11d9caa2aa..7c78791c78 100644 --- a/.gitmodules +++ b/.gitmodules @@ -20,6 +20,3 @@ [submodule "nitro-testnode"] path = nitro-testnode url = https://github.com/OffchainLabs/nitro-testnode.git -[submodule "linter/golangci-lint"] - path = linter/golangci-lint - url = https://github.com/golangci/golangci-lint diff --git a/.golangci.yml b/.golangci.yml index 47f471b5a1..e794cdb844 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -23,7 +23,6 @@ linters: - nilerr # ensure errors aren't mishandled - staticcheck # check for suspicious constructs - unused # check for unused constructs - - koanf # check for koanf configurations linters-settings: errcheck: @@ -52,8 +51,3 @@ linters-settings: disable: - shadow - fieldalignment - - custom: - koanf: - path: linter/koanf/koanf.so - description: Koanf configuration linter diff --git a/Makefile b/Makefile index 71c051087a..896bdd6a6e 100644 --- a/Makefile +++ b/Makefile @@ -304,9 +304,7 @@ contracts/test/prover/proofs/%.json: $(arbitrator_cases)/%.wasm $(arbitrator_pro # strategic rules to minimize dependency building .make/lint: $(DEP_PREDICATE) build-node-deps $(ORDER_ONLY_PREDICATE) .make - make build -C linter/golangci-lint - go build -buildmode=plugin -o linter/koanf/koanf.so linter/koanf/koanf.go - ./linter/golangci-lint/golangci-lint run --fix + go run linter/koanf/koanf.go ./... yarn --cwd contracts solhint @touch $@ diff --git a/linter/golangci-lint b/linter/golangci-lint deleted file mode 160000 index 8000abaf0e..0000000000 --- a/linter/golangci-lint +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 8000abaf0e6e28e8179864f0317349cecab47c05 diff --git a/linter/koanf/koanf.go b/linter/koanf/koanf.go index 4aaddaa495..bc94a9c20e 100644 --- a/linter/koanf/koanf.go +++ b/linter/koanf/koanf.go @@ -9,8 +9,8 @@ import ( "unicode" "github.com/fatih/structtag" - "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/singlechecker" ) func New(conf any) ([]*analysis.Analyzer, error) { @@ -114,3 +114,7 @@ func normalize(s string) string { func isAlphanumeric(c rune) bool { return unicode.IsLetter(c) || unicode.IsDigit(c) } + +func main() { + singlechecker.Main(Analyzer) +} From ed4942d38adc5afe1918740d35c3e67d1204f372 Mon Sep 17 00:00:00 2001 From: Nodar Date: Fri, 18 Aug 2023 16:02:53 +0200 Subject: [PATCH 05/22] Rename testdata to testsdata because testdata is already in .gitignore --- linter/koanf/koanf_test.go | 2 +- linter/testsdata/src/a/a.go | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) create mode 100644 linter/testsdata/src/a/a.go diff --git a/linter/koanf/koanf_test.go b/linter/koanf/koanf_test.go index 2e3e68b0f4..5582e61605 100644 --- a/linter/koanf/koanf_test.go +++ b/linter/koanf/koanf_test.go @@ -13,7 +13,7 @@ func TestAll(t *testing.T) { if err != nil { t.Fatalf("Failed to get wd: %s", err) } - testdata := filepath.Join(filepath.Dir(wd), "testdata") + testdata := filepath.Join(filepath.Dir(wd), "testsdata") res := analysistest.Run(t, testdata, analyzerForTests, "a") if cnt := countErrors(res); cnt != 1 { t.Errorf("analysistest.Run() got %v errors, expected 1", cnt) diff --git a/linter/testsdata/src/a/a.go b/linter/testsdata/src/a/a.go new file mode 100644 index 0000000000..ddf77b6ed1 --- /dev/null +++ b/linter/testsdata/src/a/a.go @@ -0,0 +1,11 @@ +package a + +type Config struct { + L2 int `koanf:"chain"` + LogLevel int `koanf:"log-level"` + LogType int `koanf:"log-type"` + Metrics int `koanf:"metrics"` + PProf int `koanf:"pprof"` + Node int `koanf:"node"` + Queue int `koanf:"queue"` +} From c93e79a45f37bc729e2d83a70991f4078964b0f4 Mon Sep 17 00:00:00 2001 From: Nodar Date: Fri, 18 Aug 2023 17:18:21 +0200 Subject: [PATCH 06/22] rename 'testsdata' to 'testdata' drop it from .gitignore --- .gitignore | 1 - linter/koanf/koanf_test.go | 2 +- linter/{testsdata => testdata}/src/a/a.go | 0 3 files changed, 1 insertion(+), 2 deletions(-) rename linter/{testsdata => testdata}/src/a/a.go (100%) diff --git a/.gitignore b/.gitignore index f0eb5c2ec3..60df842f0e 100644 --- a/.gitignore +++ b/.gitignore @@ -19,5 +19,4 @@ solgen/go/ target/ yarn-error.log local/ -testdata system_tests/test-data/* diff --git a/linter/koanf/koanf_test.go b/linter/koanf/koanf_test.go index 5582e61605..2e3e68b0f4 100644 --- a/linter/koanf/koanf_test.go +++ b/linter/koanf/koanf_test.go @@ -13,7 +13,7 @@ func TestAll(t *testing.T) { if err != nil { t.Fatalf("Failed to get wd: %s", err) } - testdata := filepath.Join(filepath.Dir(wd), "testsdata") + testdata := filepath.Join(filepath.Dir(wd), "testdata") res := analysistest.Run(t, testdata, analyzerForTests, "a") if cnt := countErrors(res); cnt != 1 { t.Errorf("analysistest.Run() got %v errors, expected 1", cnt) diff --git a/linter/testsdata/src/a/a.go b/linter/testdata/src/a/a.go similarity index 100% rename from linter/testsdata/src/a/a.go rename to linter/testdata/src/a/a.go From f95dd315b720dc149f46821691e482fabb97f938 Mon Sep 17 00:00:00 2001 From: Nodar Date: Mon, 21 Aug 2023 18:00:21 +0200 Subject: [PATCH 07/22] Implement linter detection of non-matching flag and filed name --- linter/koanf/koanf.go | 100 +++++++++++++++++++++++++++++++++++++ linter/koanf/koanf_test.go | 4 +- linter/testdata/src/a/a.go | 27 ++++++++++ 3 files changed, 129 insertions(+), 2 deletions(-) diff --git a/linter/koanf/koanf.go b/linter/koanf/koanf.go index bc94a9c20e..8dbb392cb4 100644 --- a/linter/koanf/koanf.go +++ b/linter/koanf/koanf.go @@ -51,6 +51,8 @@ func run(dryRun bool, pass *analysis.Pass) (interface{}, error) { switch v := node.(type) { case *ast.StructType: res = checkStruct(pass, v) + case *ast.FuncDecl: + res = checkFlagDefs(pass, v) default: } for _, err := range res.Errors { @@ -70,6 +72,104 @@ func run(dryRun bool, pass *analysis.Pass) (interface{}, error) { return ret, nil } +func containsFlagSet(params []*ast.Field) bool { + for _, p := range params { + se, ok := p.Type.(*ast.StarExpr) + if !ok { + continue + } + sle, ok := se.X.(*ast.SelectorExpr) + if !ok { + continue + } + if sle.Sel.Name == "FlagSet" { + return true + } + } + return false +} + +// checkFlagDefs checks flag definitions in the function. +// Result contains list of errors where flag name doesn't match field name. +func checkFlagDefs(pass *analysis.Pass, f *ast.FuncDecl) Result { + // Ignore functions that does not get flagset as parameter. + if !containsFlagSet(f.Type.Params.List) { + return Result{} + } + var res Result + for _, s := range f.Body.List { + es, ok := s.(*ast.ExprStmt) + if !ok { + continue + } + callE, ok := es.X.(*ast.CallExpr) + if !ok { + continue + } + if len(callE.Args) != 3 { + continue + } + sl, ok := extractStrLit(callE.Args[0]) + if !ok { + continue + } + s, ok := selector(callE.Args[1]) + if !ok { + continue + } + if normSL := normalize(sl); !strings.EqualFold(normSL, s) { + res.Errors = append(res.Errors, koanfError{ + Pos: pass.Fset.Position(f.Pos()), + Message: fmt.Sprintf("koanf tag name: %q doesn't match the field: %q", sl, s), + }) + } + + } + return res +} + +func selector(e ast.Expr) (string, bool) { + n, ok := e.(ast.Node) + if !ok { + return "", false + } + se, ok := n.(*ast.SelectorExpr) + if !ok { + return "", false + } + return se.Sel.Name, true +} + +// Extracts literal from expression that is either: +// - string literal or +// - sum of variable and string literal. +// E.g. +// strLitFromSum(`"max-size"`) = "max-size" +// - strLitFromSum(`prefix + ".enable"“) = ".enable". +func extractStrLit(e ast.Expr) (string, bool) { + if s, ok := strLit(e); ok { + return s, true + } + if be, ok := e.(*ast.BinaryExpr); ok { + if be.Op == token.ADD { + if s, ok := strLit(be.Y); ok { + // Drop the prefix dot. + return s[1:], true + } + } + } + return "", false +} + +func strLit(e ast.Expr) (string, bool) { + if s, ok := e.(*ast.BasicLit); ok { + if s.Kind == token.STRING { + return strings.Trim(s.Value, "\""), true + } + } + return "", false +} + func checkStruct(pass *analysis.Pass, s *ast.StructType) Result { var res Result for _, f := range s.Fields.List { diff --git a/linter/koanf/koanf_test.go b/linter/koanf/koanf_test.go index 2e3e68b0f4..e3ad5e6043 100644 --- a/linter/koanf/koanf_test.go +++ b/linter/koanf/koanf_test.go @@ -15,8 +15,8 @@ func TestAll(t *testing.T) { } testdata := filepath.Join(filepath.Dir(wd), "testdata") res := analysistest.Run(t, testdata, analyzerForTests, "a") - if cnt := countErrors(res); cnt != 1 { - t.Errorf("analysistest.Run() got %v errors, expected 1", cnt) + if cnt := countErrors(res); cnt != 3 { + t.Errorf("analysistest.Run() got %v errors, expected 3", cnt) } } diff --git a/linter/testdata/src/a/a.go b/linter/testdata/src/a/a.go index ddf77b6ed1..86b7739108 100644 --- a/linter/testdata/src/a/a.go +++ b/linter/testdata/src/a/a.go @@ -1,6 +1,11 @@ package a +import ( + "flag" +) + type Config struct { + // Field name doesn't match koanf tag. L2 int `koanf:"chain"` LogLevel int `koanf:"log-level"` LogType int `koanf:"log-type"` @@ -9,3 +14,25 @@ type Config struct { Node int `koanf:"node"` Queue int `koanf:"queue"` } + +type BatchPosterConfig struct { + Enable bool `koanf:"enable"` + MaxSize int `koanf:"max-size" reload:"hot"` +} + +// Flag names don't match field names from default config. +// Contains 2 errors. +func BatchPosterConfigAddOptions(prefix string, f *flag.FlagSet) { + f.Bool(prefix+".enabled", DefaultBatchPosterConfig.Enable, "enable posting batches to l1") + f.Int("max-sz", DefaultBatchPosterConfig.MaxSize, "maximum batch size") +} + +func ConfigAddOptions(prefix string, f *flag.FlagSet) { + f.Bool(prefix+".enable", DefaultBatchPosterConfig.Enable, "enable posting batches to l1") + f.Int("max-size", DefaultBatchPosterConfig.MaxSize, "maximum batch size") +} + +var DefaultBatchPosterConfig = BatchPosterConfig{ + Enable: false, + MaxSize: 100000, +} From 121cc8bfee933c856230b9f11d4d9fdb40a4cb0f Mon Sep 17 00:00:00 2001 From: Nodar Date: Wed, 23 Aug 2023 15:04:26 +0200 Subject: [PATCH 08/22] Merge with mater --- go.mod | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/go.mod b/go.mod index 2cae3a9cf2..10991bc3a2 100644 --- a/go.mod +++ b/go.mod @@ -33,6 +33,7 @@ require ( github.com/spf13/pflag v1.0.5 github.com/wealdtech/go-merkletree v1.0.0 golang.org/x/term v0.6.0 + golang.org/x/tools v0.7.0 gopkg.in/natefinch/lumberjack.v2 v2.0.0 ) @@ -258,7 +259,6 @@ require ( go4.org v0.0.0-20200411211856-f5505b9728dd // indirect golang.org/x/exp v0.0.0-20230206171751-46f607a40771 // indirect golang.org/x/mod v0.9.0 // indirect - golang.org/x/tools v0.7.0 // indirect golang.org/x/xerrors v0.0.0-20220907171357-04be3eba64a2 // indirect google.golang.org/genproto v0.0.0-20211118181313-81c1377c94b1 // indirect google.golang.org/grpc v1.46.0 // indirect @@ -312,7 +312,7 @@ require ( golang.org/x/crypto v0.6.0 golang.org/x/net v0.8.0 // indirect golang.org/x/sync v0.1.0 // indirect - golang.org/x/sys v0.6.0 + golang.org/x/sys v0.7.0 golang.org/x/text v0.8.0 // indirect golang.org/x/time v0.0.0-20220922220347-f3bd1da661af // indirect gopkg.in/natefinch/npipe.v2 v2.0.0-20160621034901-c1b8fa8bdcce // indirect From f6f659e7b2513ab48f0a276340676636350d98f0 Mon Sep 17 00:00:00 2001 From: Nodar Date: Wed, 30 Aug 2023 18:04:20 +0200 Subject: [PATCH 09/22] Implement legacy storage encoding with hot-reload flag to change encoding strategy on the fly --- arbnode/dataposter/data_poster.go | 24 +++-- arbnode/dataposter/leveldb/leveldb.go | 26 ++--- arbnode/dataposter/redis/redisstorage.go | 28 +++--- arbnode/dataposter/slice/slicestorage.go | 22 ++--- arbnode/dataposter/storage/storage.go | 117 +++++++++++++++++++++++ arbnode/dataposter/storage_test.go | 64 ++++++++----- 6 files changed, 202 insertions(+), 79 deletions(-) diff --git a/arbnode/dataposter/data_poster.go b/arbnode/dataposter/data_poster.go index b1db655d71..adfde88f74 100644 --- a/arbnode/dataposter/data_poster.go +++ b/arbnode/dataposter/data_poster.go @@ -101,17 +101,23 @@ func NewDataPoster(db ethdb.Database, headerReader *headerreader.HeaderReader, a initConfig.UseNoOpStorage = true log.Info("Disabling data poster storage, as parent chain appears to be an Arbitrum chain without a mempool") } + encF := func() storage.EncoderDecoderInterface { + if config().LegacyStorageEncoding { + return &storage.LegacyEncoderDecoder{} + } + return &storage.EncoderDecoder{} + } var queue QueueStorage switch { case initConfig.UseNoOpStorage: queue = &noop.Storage{} case initConfig.UseLevelDB: - queue = leveldb.New(db) + queue = leveldb.New(db, encF) case redisClient == nil: - queue = slice.NewStorage() + queue = slice.NewStorage(encF) default: var err error - queue, err = redisstorage.NewStorage(redisClient, "data-poster.queue", &initConfig.RedisSigner) + queue, err = redisstorage.NewStorage(redisClient, "data-poster.queue", &initConfig.RedisSigner, encF) if err != nil { return nil, err } @@ -174,7 +180,7 @@ func (p *DataPoster) getNextNonceAndMaybeMeta(ctx context.Context) (uint64, []by } lastQueueItem, err := p.queue.FetchLast(ctx) if err != nil { - return 0, nil, false, err + return 0, nil, false, fmt.Errorf("fetching last element from queue: %w", err) } if lastQueueItem != nil { nextNonce := lastQueueItem.Data.Nonce + 1 @@ -364,7 +370,10 @@ func (p *DataPoster) saveTx(ctx context.Context, prevTx, newTx *storage.QueuedTr if prevTx != nil && prevTx.Data.Nonce != newTx.Data.Nonce { return fmt.Errorf("prevTx nonce %v doesn't match newTx nonce %v", prevTx.Data.Nonce, newTx.Data.Nonce) } - return p.queue.Put(ctx, newTx.Data.Nonce, prevTx, newTx) + if err := p.queue.Put(ctx, newTx.Data.Nonce, prevTx, newTx); err != nil { + return fmt.Errorf("putting new tx in the queue: %w", err) + } + return nil } func (p *DataPoster) sendTx(ctx context.Context, prevTx *storage.QueuedTransaction, newTx *storage.QueuedTransaction) error { @@ -546,7 +555,7 @@ func (p *DataPoster) Start(ctxIn context.Context) { // replacing them by fee. queueContents, err := p.queue.FetchContents(ctx, unconfirmedNonce, maxTxsToRbf) if err != nil { - log.Error("Failed to get tx queue contents", "err", err) + log.Error("Failed to fetch tx queue contents", "err", err) return minWait } for index, tx := range queueContents { @@ -616,6 +625,7 @@ type DataPosterConfig struct { AllocateMempoolBalance bool `koanf:"allocate-mempool-balance" reload:"hot"` UseLevelDB bool `koanf:"use-leveldb"` UseNoOpStorage bool `koanf:"use-noop-storage"` + LegacyStorageEncoding bool `koanf:"legacy-storage-encoding" reload:"hot"` } // ConfigFetcher function type is used instead of directly passing config so @@ -636,6 +646,7 @@ func DataPosterConfigAddOptions(prefix string, f *pflag.FlagSet) { f.Bool(prefix+".allocate-mempool-balance", DefaultDataPosterConfig.AllocateMempoolBalance, "if true, don't put transactions in the mempool that spend a total greater than the batch poster's balance") f.Bool(prefix+".use-leveldb", DefaultDataPosterConfig.UseLevelDB, "uses leveldb when enabled") f.Bool(prefix+".use-noop-storage", DefaultDataPosterConfig.UseNoOpStorage, "uses noop storage, it doesn't store anything") + f.Bool(prefix+".legacy-storage-encoding", DefaultDataPosterConfig.LegacyStorageEncoding, "encodes items in a legacy way (as it was before dropping generics)") signature.SimpleHmacConfigAddOptions(prefix+".redis-signer", f) } @@ -651,6 +662,7 @@ var DefaultDataPosterConfig = DataPosterConfig{ AllocateMempoolBalance: true, UseLevelDB: false, UseNoOpStorage: false, + LegacyStorageEncoding: true, } var DefaultDataPosterConfigForValidator = func() DataPosterConfig { diff --git a/arbnode/dataposter/leveldb/leveldb.go b/arbnode/dataposter/leveldb/leveldb.go index e41a8665a6..cfb34b04f7 100644 --- a/arbnode/dataposter/leveldb/leveldb.go +++ b/arbnode/dataposter/leveldb/leveldb.go @@ -12,14 +12,14 @@ import ( "github.com/ethereum/go-ethereum/ethdb" "github.com/ethereum/go-ethereum/ethdb/memorydb" - "github.com/ethereum/go-ethereum/rlp" "github.com/offchainlabs/nitro/arbnode/dataposter/storage" "github.com/syndtr/goleveldb/leveldb" ) // Storage implements leveldb based storage for batch poster. type Storage struct { - db ethdb.Database + db ethdb.Database + encDec storage.EncoderDecoderF } var ( @@ -31,16 +31,8 @@ var ( countKey = []byte(".count_key") ) -func New(db ethdb.Database) *Storage { - return &Storage{db: db} -} - -func (s *Storage) decodeItem(data []byte) (*storage.QueuedTransaction, error) { - var item storage.QueuedTransaction - if err := rlp.DecodeBytes(data, &item); err != nil { - return nil, fmt.Errorf("decoding item: %w", err) - } - return &item, nil +func New(db ethdb.Database, enc storage.EncoderDecoderF) *Storage { + return &Storage{db: db, encDec: enc} } func idxToKey(idx uint64) []byte { @@ -55,7 +47,7 @@ func (s *Storage) FetchContents(_ context.Context, startingIndex uint64, maxResu if !it.Next() { break } - item, err := s.decodeItem(it.Value()) + item, err := s.encDec().Decode(it.Value()) if err != nil { return nil, err } @@ -84,7 +76,7 @@ func (s *Storage) FetchLast(ctx context.Context) (*storage.QueuedTransaction, er if err != nil { return nil, err } - return s.decodeItem(val) + return s.encDec().Decode(val) } func (s *Storage) Prune(ctx context.Context, until uint64) error { @@ -117,7 +109,7 @@ func (s *Storage) valueAt(_ context.Context, key []byte) ([]byte, error) { val, err := s.db.Get(key) if err != nil { if isErrNotFound(err) { - return rlp.EncodeToBytes((*storage.QueuedTransaction)(nil)) + return s.encDec().Encode((*storage.QueuedTransaction)(nil)) } return nil, err } @@ -130,14 +122,14 @@ func (s *Storage) Put(ctx context.Context, index uint64, prev, new *storage.Queu if err != nil { return err } - prevEnc, err := rlp.EncodeToBytes(prev) + prevEnc, err := s.encDec().Encode(prev) if err != nil { return fmt.Errorf("encoding previous item: %w", err) } if !bytes.Equal(stored, prevEnc) { return fmt.Errorf("replacing different item than expected at index: %v, stored: %v, prevEnc: %v", index, stored, prevEnc) } - newEnc, err := rlp.EncodeToBytes(new) + newEnc, err := s.encDec().Encode(new) if err != nil { return fmt.Errorf("encoding new item: %w", err) } diff --git a/arbnode/dataposter/redis/redisstorage.go b/arbnode/dataposter/redis/redisstorage.go index e6fe666c69..5123cea154 100644 --- a/arbnode/dataposter/redis/redisstorage.go +++ b/arbnode/dataposter/redis/redisstorage.go @@ -9,7 +9,6 @@ import ( "errors" "fmt" - "github.com/ethereum/go-ethereum/rlp" "github.com/go-redis/redis/v8" "github.com/offchainlabs/nitro/arbnode/dataposter/storage" "github.com/offchainlabs/nitro/util/signature" @@ -23,14 +22,15 @@ type Storage struct { client redis.UniversalClient signer *signature.SimpleHmac key string + encDec storage.EncoderDecoderF } -func NewStorage(client redis.UniversalClient, key string, signerConf *signature.SimpleHmacConfig) (*Storage, error) { +func NewStorage(client redis.UniversalClient, key string, signerConf *signature.SimpleHmacConfig, enc storage.EncoderDecoderF) (*Storage, error) { signer, err := signature.NewSimpleHmac(signerConf) if err != nil { return nil, err } - return &Storage{client, signer, key}, nil + return &Storage{client, signer, key, enc}, nil } func joinHmacMsg(msg []byte, sig []byte) ([]byte, error) { @@ -65,16 +65,15 @@ func (s *Storage) FetchContents(ctx context.Context, startingIndex uint64, maxRe } var items []*storage.QueuedTransaction for _, itemString := range itemStrings { - var item storage.QueuedTransaction data, err := s.peelVerifySignature([]byte(itemString)) if err != nil { return nil, err } - err = rlp.DecodeBytes(data, &item) + item, err := s.encDec().Decode(data) if err != nil { return nil, err } - items = append(items, &item) + items = append(items, item) } return items, nil } @@ -95,16 +94,15 @@ func (s *Storage) FetchLast(ctx context.Context) (*storage.QueuedTransaction, er } var ret *storage.QueuedTransaction if len(itemStrings) > 0 { - var item storage.QueuedTransaction data, err := s.peelVerifySignature([]byte(itemStrings[0])) if err != nil { return nil, err } - err = rlp.DecodeBytes(data, &item) + item, err := s.encDec().Decode(data) if err != nil { return nil, err } - ret = &item + ret = item } return ret, nil } @@ -144,21 +142,20 @@ func (s *Storage) Put(ctx context.Context, index uint64, prev, new *storage.Queu if err != nil { return fmt.Errorf("failed to validate item already in redis at index%v: %w", index, err) } - prevItemEncoded, err := rlp.EncodeToBytes(prev) + prevItemEncoded, err := s.encDec().Encode(prev) if err != nil { return err } if !bytes.Equal(verifiedItem, prevItemEncoded) { return fmt.Errorf("%w: replacing different item than expected at index %v", storage.ErrStorageRace, index) } - err = pipe.ZRem(ctx, s.key, haveItems[0]).Err() - if err != nil { + if err := pipe.ZRem(ctx, s.key, haveItems[0]).Err(); err != nil { return err } } else { return fmt.Errorf("expected only one return value for Put but got %v", len(haveItems)) } - newItemEncoded, err := rlp.EncodeToBytes(*new) + newItemEncoded, err := s.encDec().Encode(new) if err != nil { return err } @@ -170,11 +167,10 @@ func (s *Storage) Put(ctx context.Context, index uint64, prev, new *storage.Queu if err != nil { return err } - err = pipe.ZAdd(ctx, s.key, &redis.Z{ + if err := pipe.ZAdd(ctx, s.key, &redis.Z{ Score: float64(index), Member: string(signedItem), - }).Err() - if err != nil { + }).Err(); err != nil { return err } _, err = pipe.Exec(ctx) diff --git a/arbnode/dataposter/slice/slicestorage.go b/arbnode/dataposter/slice/slicestorage.go index 6eda5ca9a3..04286df411 100644 --- a/arbnode/dataposter/slice/slicestorage.go +++ b/arbnode/dataposter/slice/slicestorage.go @@ -9,25 +9,17 @@ import ( "errors" "fmt" - "github.com/ethereum/go-ethereum/rlp" "github.com/offchainlabs/nitro/arbnode/dataposter/storage" ) type Storage struct { firstNonce uint64 queue [][]byte + encDec func() storage.EncoderDecoderInterface } -func NewStorage() *Storage { - return &Storage{} -} - -func (s *Storage) decodeItem(data []byte) (*storage.QueuedTransaction, error) { - var item storage.QueuedTransaction - if err := rlp.DecodeBytes(data, &item); err != nil { - return nil, fmt.Errorf("decoding item: %w", err) - } - return &item, nil +func NewStorage(encDec func() storage.EncoderDecoderInterface) *Storage { + return &Storage{encDec: encDec} } func (s *Storage) FetchContents(_ context.Context, startingIndex uint64, maxResults uint64) ([]*storage.QueuedTransaction, error) { @@ -43,7 +35,7 @@ func (s *Storage) FetchContents(_ context.Context, startingIndex uint64, maxResu } var res []*storage.QueuedTransaction for _, r := range txs { - item, err := s.decodeItem(r) + item, err := s.encDec().Decode(r) if err != nil { return nil, err } @@ -56,7 +48,7 @@ func (s *Storage) FetchLast(context.Context) (*storage.QueuedTransaction, error) if len(s.queue) == 0 { return nil, nil } - return s.decodeItem(s.queue[len(s.queue)-1]) + return s.encDec().Decode(s.queue[len(s.queue)-1]) } func (s *Storage) Prune(_ context.Context, until uint64) error { @@ -73,7 +65,7 @@ func (s *Storage) Put(_ context.Context, index uint64, prev, new *storage.Queued if new == nil { return fmt.Errorf("tried to insert nil item at index %v", index) } - newEnc, err := rlp.EncodeToBytes(new) + newEnc, err := s.encDec().Encode(new) if err != nil { return fmt.Errorf("encoding new item: %w", err) } @@ -93,7 +85,7 @@ func (s *Storage) Put(_ context.Context, index uint64, prev, new *storage.Queued if queueIdx > len(s.queue) { return fmt.Errorf("attempted to set out-of-bounds index %v in queue starting at %v of length %v", index, s.firstNonce, len(s.queue)) } - prevEnc, err := rlp.EncodeToBytes(prev) + prevEnc, err := s.encDec().Encode(prev) if err != nil { return fmt.Errorf("encoding previous item: %w", err) } diff --git a/arbnode/dataposter/storage/storage.go b/arbnode/dataposter/storage/storage.go index 174ab131ac..57085c4fb1 100644 --- a/arbnode/dataposter/storage/storage.go +++ b/arbnode/dataposter/storage/storage.go @@ -2,9 +2,12 @@ package storage import ( "errors" + "fmt" "time" "github.com/ethereum/go-ethereum/core/types" + "github.com/ethereum/go-ethereum/rlp" + "github.com/offchainlabs/nitro/arbutil" ) var ( @@ -24,3 +27,117 @@ type QueuedTransaction struct { Created time.Time // may be earlier than the tx was given to the tx poster NextReplacement time.Time } + +// LegacyQueuedTransaction is used for backwards compatibility. +// Before https://github.com/OffchainLabs/nitro/pull/1773: the queuedTransaction +// looked like this and was rlp encoded directly. After the pr, we are store +// rlp encoding of Meta into queuedTransaction and rlp encoding it once more +// to store it. +type LegacyQueuedTransaction struct { + FullTx *types.Transaction `rlp:"nil"` + Data types.DynamicFeeTx + Meta BatchPosterPosition + Sent bool + Created time.Time // may be earlier than the tx was given to the tx poster + NextReplacement time.Time +} + +// This is also for legacy reason. Since Batchposter is in arbnode package, +// we can't refer to BatchPosterPosition type there even if we export it (that +// would create cyclic dependency). +// Ideally we'll factor out Batch Poster from arbnode into separate package +// and BatchPosterPosition into another separate package as well. +// For the sake of minimal refactoring, that struct is duplicated here. +type BatchPosterPosition struct { + MessageCount arbutil.MessageIndex + DelayedMessageCount uint64 + NextSeqNum uint64 +} + +func DecodeLegacyQueuedTransaction(data []byte) (*LegacyQueuedTransaction, error) { + var val LegacyQueuedTransaction + if err := rlp.DecodeBytes(data, &val); err != nil { + return nil, fmt.Errorf("decoding legacy queued transaction: %w", err) + } + return &val, nil +} + +func LegacyToQueuedTransaction(legacyQT *LegacyQueuedTransaction) (*QueuedTransaction, error) { + meta, err := rlp.EncodeToBytes(legacyQT.Meta) + if err != nil { + return nil, fmt.Errorf("converting legacy to queued transaction: %w", err) + } + return &QueuedTransaction{ + FullTx: legacyQT.FullTx, + Data: legacyQT.Data, + Meta: meta, + Sent: legacyQT.Sent, + Created: legacyQT.Created, + NextReplacement: legacyQT.NextReplacement, + }, nil +} + +func QueuedTransactionToLegacy(qt *QueuedTransaction) (*LegacyQueuedTransaction, error) { + if qt == nil { + return nil, nil + } + var meta BatchPosterPosition + if qt.Meta != nil { + if err := rlp.DecodeBytes(qt.Meta, &meta); err != nil { + return nil, fmt.Errorf("converting queued transaction to legacy: %w", err) + } + } + return &LegacyQueuedTransaction{ + FullTx: qt.FullTx, + Data: qt.Data, + Meta: meta, + Sent: qt.Sent, + Created: qt.Created, + NextReplacement: qt.NextReplacement, + }, nil +} + +type EncoderDecoder struct{} + +func (e *EncoderDecoder) Encode(qt *QueuedTransaction) ([]byte, error) { + return rlp.EncodeToBytes(qt) +} + +func (e *EncoderDecoder) Decode(data []byte) (*QueuedTransaction, error) { + var item QueuedTransaction + if err := rlp.DecodeBytes(data, &item); err != nil { + return nil, fmt.Errorf("decoding item: %w", err) + } + return &item, nil +} + +type LegacyEncoderDecoder struct{} + +func (e *LegacyEncoderDecoder) Encode(qt *QueuedTransaction) ([]byte, error) { + legacyQt, err := QueuedTransactionToLegacy(qt) + if err != nil { + return nil, fmt.Errorf("encoding legacy item: %w", err) + } + return rlp.EncodeToBytes(legacyQt) +} + +func (le *LegacyEncoderDecoder) Decode(data []byte) (*QueuedTransaction, error) { + val, err := DecodeLegacyQueuedTransaction(data) + if err != nil { + return nil, fmt.Errorf("decoding legacy item: %w", err) + } + return LegacyToQueuedTransaction(val) +} + +// Typically interfaces belong to where they are being used, not at implementing +// site, but this is used in all storages (besides no-op) and all of them +// require all the functions for this interface. +type EncoderDecoderInterface interface { + Encode(*QueuedTransaction) ([]byte, error) + Decode([]byte) (*QueuedTransaction, error) +} + +// EncoderDecoderF is a function type that returns encoder/decoder interface. +// This is needed to implement hot-reloading flag to switch encoding/decoding +// strategy on the fly. +type EncoderDecoderF func() EncoderDecoderInterface diff --git a/arbnode/dataposter/storage_test.go b/arbnode/dataposter/storage_test.go index 2424ac0845..eac05502be 100644 --- a/arbnode/dataposter/storage_test.go +++ b/arbnode/dataposter/storage_test.go @@ -8,6 +8,7 @@ import ( "github.com/ethereum/go-ethereum/core/rawdb" "github.com/ethereum/go-ethereum/core/types" + "github.com/ethereum/go-ethereum/rlp" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "github.com/offchainlabs/nitro/arbnode/dataposter/leveldb" @@ -27,36 +28,41 @@ var ignoreData = cmp.Options{ cmpopts.IgnoreFields(types.Transaction{}, "hash", "size", "from"), } -func newLevelDBStorage(t *testing.T) *leveldb.Storage { +func newLevelDBStorage(t *testing.T, encF storage.EncoderDecoderF) *leveldb.Storage { t.Helper() db, err := rawdb.NewLevelDBDatabase(path.Join(t.TempDir(), "level.db"), 0, 0, "default", false) if err != nil { t.Fatalf("NewLevelDBDatabase() unexpected error: %v", err) } - return leveldb.New(db) + return leveldb.New(db, encF) } -func newSliceStorage() *slice.Storage { - return slice.NewStorage() +func newSliceStorage(encF storage.EncoderDecoderF) *slice.Storage { + return slice.NewStorage(encF) } -func newRedisStorage(ctx context.Context, t *testing.T) *redis.Storage { +func newRedisStorage(ctx context.Context, t *testing.T, encF storage.EncoderDecoderF) *redis.Storage { t.Helper() redisUrl := redisutil.CreateTestRedis(ctx, t) client, err := redisutil.RedisClientFromURL(redisUrl) if err != nil { t.Fatalf("RedisClientFromURL(%q) unexpected error: %v", redisUrl, err) } - s, err := redis.NewStorage(client, "", &signature.TestSimpleHmacConfig) + s, err := redis.NewStorage(client, "", &signature.TestSimpleHmacConfig, encF) if err != nil { t.Fatalf("redis.NewStorage() unexpected error: %v", err) } return s } -func valueOf(i int) *storage.QueuedTransaction { +func valueOf(t *testing.T, i int) *storage.QueuedTransaction { + t.Helper() + meta, err := rlp.EncodeToBytes(storage.BatchPosterPosition{DelayedMessageCount: uint64(i)}) + if err != nil { + t.Fatalf("Encoding batch poster position, error: %v", err) + } return &storage.QueuedTransaction{ - Meta: []byte{byte(i)}, + Meta: meta, Data: types.DynamicFeeTx{ ChainID: big.NewInt(int64(i)), Nonce: uint64(i), @@ -73,10 +79,10 @@ func valueOf(i int) *storage.QueuedTransaction { } } -func values(from, to int) []*storage.QueuedTransaction { +func values(t *testing.T, from, to int) []*storage.QueuedTransaction { var res []*storage.QueuedTransaction for i := from; i <= to; i++ { - res = append(res, valueOf(i)) + res = append(res, valueOf(t, i)) } return res } @@ -85,7 +91,7 @@ func values(from, to int) []*storage.QueuedTransaction { func initStorage(ctx context.Context, t *testing.T, s QueueStorage) QueueStorage { t.Helper() for i := 0; i < 20; i++ { - if err := s.Put(ctx, uint64(i), nil, valueOf(i)); err != nil { + if err := s.Put(ctx, uint64(i), nil, valueOf(t, i)); err != nil { t.Fatalf("Error putting a key/value: %v", err) } } @@ -95,10 +101,18 @@ func initStorage(ctx context.Context, t *testing.T, s QueueStorage) QueueStorage // Returns a map of all empty storages. func storages(t *testing.T) map[string]QueueStorage { t.Helper() + f := func(enc storage.EncoderDecoderInterface) storage.EncoderDecoderF { + return func() storage.EncoderDecoderInterface { + return enc + } + } return map[string]QueueStorage{ - "levelDB": newLevelDBStorage(t), - "slice": newSliceStorage(), - "redis": newRedisStorage(context.Background(), t), + "levelDBLegacy": newLevelDBStorage(t, f(&storage.LegacyEncoderDecoder{})), + "sliceLegacy": newSliceStorage(f(&storage.LegacyEncoderDecoder{})), + "redisLegacy": newRedisStorage(context.Background(), t, f(&storage.LegacyEncoderDecoder{})), + "levelDB": newLevelDBStorage(t, f(&storage.EncoderDecoder{})), + "slice": newSliceStorage(f(&storage.EncoderDecoder{})), + "redis": newRedisStorage(context.Background(), t, f(&storage.EncoderDecoder{})), } } @@ -125,13 +139,13 @@ func TestFetchContents(t *testing.T) { desc: "sequence with single digits", startIdx: 5, maxResults: 3, - want: values(5, 7), + want: values(t, 5, 7), }, { desc: "corner case of single element", startIdx: 0, maxResults: 1, - want: values(0, 0), + want: values(t, 0, 0), }, { desc: "no elements", @@ -143,13 +157,13 @@ func TestFetchContents(t *testing.T) { desc: "sequence with variable number of digits", startIdx: 9, maxResults: 3, - want: values(9, 11), + want: values(t, 9, 11), }, { desc: "max results goes over the last element", startIdx: 13, maxResults: 10, - want: values(13, 19), + want: values(t, 13, 19), }, } { t.Run(name+"_"+tc.desc, func(t *testing.T) { @@ -171,7 +185,7 @@ func TestLast(t *testing.T) { t.Run(name, func(t *testing.T) { ctx := context.Background() for i := 0; i < cnt; i++ { - val := valueOf(i) + val := valueOf(t, i) if err := s.Put(ctx, uint64(i), nil, val); err != nil { t.Fatalf("Error putting a key/value: %v", err) } @@ -185,12 +199,12 @@ func TestLast(t *testing.T) { } }) - last := valueOf(cnt - 1) + last := valueOf(t, cnt-1) t.Run(name+"_update_entries", func(t *testing.T) { ctx := context.Background() for i := 0; i < cnt-1; i++ { - prev := valueOf(i) - newVal := valueOf(cnt + i) + prev := valueOf(t, i) + newVal := valueOf(t, cnt+i) if err := s.Put(ctx, uint64(i), prev, newVal); err != nil { t.Fatalf("Error putting a key/value: %v, prev: %v, new: %v", err, prev, newVal) } @@ -227,17 +241,17 @@ func TestPrune(t *testing.T) { { desc: "prune all but one", pruneFrom: 19, - want: values(19, 19), + want: values(t, 19, 19), }, { desc: "pruning first element", pruneFrom: 1, - want: values(1, 19), + want: values(t, 1, 19), }, { desc: "pruning first 11 elements", pruneFrom: 11, - want: values(11, 19), + want: values(t, 11, 19), }, { desc: "pruning from higher than biggest index", From bb08e9d3fb5a5424f7e5885e411733978a2df736 Mon Sep 17 00:00:00 2001 From: Nodar Date: Wed, 30 Aug 2023 18:51:23 +0200 Subject: [PATCH 10/22] Use new encoding for slicestorage, attempt both decodings no matter what encoding is enabled --- arbnode/dataposter/data_poster.go | 2 +- arbnode/dataposter/storage/storage.go | 29 ++++++++++++++++++--------- 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/arbnode/dataposter/data_poster.go b/arbnode/dataposter/data_poster.go index adfde88f74..a289a17626 100644 --- a/arbnode/dataposter/data_poster.go +++ b/arbnode/dataposter/data_poster.go @@ -114,7 +114,7 @@ func NewDataPoster(db ethdb.Database, headerReader *headerreader.HeaderReader, a case initConfig.UseLevelDB: queue = leveldb.New(db, encF) case redisClient == nil: - queue = slice.NewStorage(encF) + queue = slice.NewStorage(func() storage.EncoderDecoderInterface { return &storage.EncoderDecoder{} }) default: var err error queue, err = redisstorage.NewStorage(redisClient, "data-poster.queue", &initConfig.RedisSigner, encF) diff --git a/arbnode/dataposter/storage/storage.go b/arbnode/dataposter/storage/storage.go index 57085c4fb1..a132f375e5 100644 --- a/arbnode/dataposter/storage/storage.go +++ b/arbnode/dataposter/storage/storage.go @@ -6,6 +6,7 @@ import ( "time" "github.com/ethereum/go-ethereum/core/types" + "github.com/ethereum/go-ethereum/log" "github.com/ethereum/go-ethereum/rlp" "github.com/offchainlabs/nitro/arbutil" ) @@ -97,6 +98,22 @@ func QueuedTransactionToLegacy(qt *QueuedTransaction) (*LegacyQueuedTransaction, }, nil } +// Decode tries to decode QueuedTransaction, if that fails it tries to decode +// into legacy queued transaction and converts to queued +func decode(data []byte) (*QueuedTransaction, error) { + var item QueuedTransaction + if err := rlp.DecodeBytes(data, &item); err != nil { + log.Warn("Failed to decode QueuedTransaction, attempting to decide legacy queued transaction", "error", err) + val, err := DecodeLegacyQueuedTransaction(data) + if err != nil { + return nil, fmt.Errorf("decoding legacy item: %w", err) + } + return LegacyToQueuedTransaction(val) + } + return &item, nil + +} + type EncoderDecoder struct{} func (e *EncoderDecoder) Encode(qt *QueuedTransaction) ([]byte, error) { @@ -104,11 +121,7 @@ func (e *EncoderDecoder) Encode(qt *QueuedTransaction) ([]byte, error) { } func (e *EncoderDecoder) Decode(data []byte) (*QueuedTransaction, error) { - var item QueuedTransaction - if err := rlp.DecodeBytes(data, &item); err != nil { - return nil, fmt.Errorf("decoding item: %w", err) - } - return &item, nil + return decode(data) } type LegacyEncoderDecoder struct{} @@ -122,11 +135,7 @@ func (e *LegacyEncoderDecoder) Encode(qt *QueuedTransaction) ([]byte, error) { } func (le *LegacyEncoderDecoder) Decode(data []byte) (*QueuedTransaction, error) { - val, err := DecodeLegacyQueuedTransaction(data) - if err != nil { - return nil, fmt.Errorf("decoding legacy item: %w", err) - } - return LegacyToQueuedTransaction(val) + return decode(data) } // Typically interfaces belong to where they are being used, not at implementing From 2e6d59ac0c3bfa6679011c974d37fdd16fad2992 Mon Sep 17 00:00:00 2001 From: Nodar Date: Wed, 30 Aug 2023 18:52:20 +0200 Subject: [PATCH 11/22] drop empty line --- arbnode/dataposter/storage/storage.go | 1 - 1 file changed, 1 deletion(-) diff --git a/arbnode/dataposter/storage/storage.go b/arbnode/dataposter/storage/storage.go index a132f375e5..ed848b9b7d 100644 --- a/arbnode/dataposter/storage/storage.go +++ b/arbnode/dataposter/storage/storage.go @@ -111,7 +111,6 @@ func decode(data []byte) (*QueuedTransaction, error) { return LegacyToQueuedTransaction(val) } return &item, nil - } type EncoderDecoder struct{} From 06930b67bf24630f002abbe523c2ae9e3fd71362 Mon Sep 17 00:00:00 2001 From: Lee Bousfield Date: Wed, 30 Aug 2023 12:56:15 -0600 Subject: [PATCH 12/22] Fix pollForReverts channel reading --- arbnode/batch_poster.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arbnode/batch_poster.go b/arbnode/batch_poster.go index 429701be7e..7efa2b3f73 100644 --- a/arbnode/batch_poster.go +++ b/arbnode/batch_poster.go @@ -309,8 +309,8 @@ func (b *BatchPoster) pollForReverts(ctx context.Context) { // - polling is through context, or // - we see a transaction in the block from dataposter that was reverted. select { - case h, closed := <-headerCh: - if closed { + case h, ok := <-headerCh: + if !ok { log.Info("L1 headers channel has been closed") return } From 370ac231089a56649372e7bbe21ce3c26cefda80 Mon Sep 17 00:00:00 2001 From: Lee Bousfield Date: Wed, 30 Aug 2023 20:43:12 -0600 Subject: [PATCH 13/22] Fix off by 1 in validator logging --- staker/block_validator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/staker/block_validator.go b/staker/block_validator.go index f04b852041..94bc2a0806 100644 --- a/staker/block_validator.go +++ b/staker/block_validator.go @@ -597,7 +597,7 @@ func (v *BlockValidator) iterativeValidationPrint(ctx context.Context) time.Dura var batchMsgs arbutil.MessageIndex var printedCount int64 if validated.GlobalState.Batch > 0 { - batchMsgs, err = v.inboxTracker.GetBatchMessageCount(validated.GlobalState.Batch) + batchMsgs, err = v.inboxTracker.GetBatchMessageCount(validated.GlobalState.Batch - 1) } if err != nil { printedCount = -1 From a7e26b2c038471bcd11831fbcc517238114efbb9 Mon Sep 17 00:00:00 2001 From: Nodar Date: Thu, 31 Aug 2023 15:30:04 +0200 Subject: [PATCH 14/22] Drop normalize method in koanf.go, ignore case when comparing tag and a field --- linter/koanf/koanf.go | 22 +--------------------- 1 file changed, 1 insertion(+), 21 deletions(-) diff --git a/linter/koanf/koanf.go b/linter/koanf/koanf.go index bc94a9c20e..2127fb23b0 100644 --- a/linter/koanf/koanf.go +++ b/linter/koanf/koanf.go @@ -6,7 +6,6 @@ import ( "go/token" "reflect" "strings" - "unicode" "github.com/fatih/structtag" "golang.org/x/tools/go/analysis" @@ -84,7 +83,7 @@ func checkStruct(pass *analysis.Pass, s *ast.StructType) Result { if err != nil { continue } - tagName := normalize(tag.Name) + tagName := strings.ReplaceAll(tag.Name, "-", "") fieldName := f.Names[0].Name if !strings.EqualFold(tagName, fieldName) { res.Errors = append(res.Errors, koanfError{ @@ -96,25 +95,6 @@ func checkStruct(pass *analysis.Pass, s *ast.StructType) Result { return res } -func normalize(s string) string { - ans := s[:1] - for i := 1; i < len(s); i++ { - c := rune(s[i]) - if !isAlphanumeric(c) { - continue - } - if !isAlphanumeric(rune(s[i-1])) && unicode.IsLower(c) { - c = unicode.ToUpper(c) - } - ans += string(c) - } - return ans -} - -func isAlphanumeric(c rune) bool { - return unicode.IsLetter(c) || unicode.IsDigit(c) -} - func main() { singlechecker.Main(Analyzer) } From 8cb2606040a993d7fd1f6cf08e5af842ff1b2d1d Mon Sep 17 00:00:00 2001 From: ganeshvanahalli Date: Thu, 31 Aug 2023 10:49:42 -0500 Subject: [PATCH 15/22] add new type Uint64OrHex --- go-ethereum | 2 +- system_tests/conditionaltx_test.go | 29 ++++++++++++++--------------- 2 files changed, 15 insertions(+), 16 deletions(-) diff --git a/go-ethereum b/go-ethereum index c905292f8a..f8363dc42d 160000 --- a/go-ethereum +++ b/go-ethereum @@ -1 +1 @@ -Subproject commit c905292f8af601f7fca261e65a7d4bc144261e29 +Subproject commit f8363dc42d6cf8dbd76aed2550207a90919e88ee diff --git a/system_tests/conditionaltx_test.go b/system_tests/conditionaltx_test.go index c65103694a..b758ba1807 100644 --- a/system_tests/conditionaltx_test.go +++ b/system_tests/conditionaltx_test.go @@ -16,7 +16,6 @@ import ( "github.com/ethereum/go-ethereum/arbitrum" "github.com/ethereum/go-ethereum/arbitrum_types" "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/crypto" "github.com/ethereum/go-ethereum/rpc" @@ -103,23 +102,23 @@ func getOptions(address common.Address, rootHash common.Hash, slotValueMap map[c } func getFulfillableBlockTimeLimits(t *testing.T, blockNumber uint64, timestamp uint64) []*arbitrum_types.ConditionalOptions { - future := hexutil.Uint64(timestamp + 30) - past := hexutil.Uint64(timestamp - 1) - futureBlockNumber := hexutil.Uint64(blockNumber + 1000) - currentBlockNumber := hexutil.Uint64(blockNumber) + future := common.Uint64OrHex(timestamp + 30) + past := common.Uint64OrHex(timestamp - 1) + futureBlockNumber := common.Uint64OrHex(blockNumber + 1000) + currentBlockNumber := common.Uint64OrHex(blockNumber) return getBlockTimeLimits(t, currentBlockNumber, futureBlockNumber, past, future) } func getUnfulfillableBlockTimeLimits(t *testing.T, blockNumber uint64, timestamp uint64) []*arbitrum_types.ConditionalOptions { - future := hexutil.Uint64(timestamp + 30) - past := hexutil.Uint64(timestamp - 1) - futureBlockNumber := hexutil.Uint64(blockNumber + 1000) - previousBlockNumber := hexutil.Uint64(blockNumber - 1) + future := common.Uint64OrHex(timestamp + 30) + past := common.Uint64OrHex(timestamp - 1) + futureBlockNumber := common.Uint64OrHex(blockNumber + 1000) + previousBlockNumber := common.Uint64OrHex(blockNumber - 1) // skip first empty options return getBlockTimeLimits(t, futureBlockNumber, previousBlockNumber, future, past)[1:] } -func getBlockTimeLimits(t *testing.T, blockMin, blockMax hexutil.Uint64, timeMin, timeMax hexutil.Uint64) []*arbitrum_types.ConditionalOptions { +func getBlockTimeLimits(t *testing.T, blockMin, blockMax common.Uint64OrHex, timeMin, timeMax common.Uint64OrHex) []*arbitrum_types.ConditionalOptions { basic := []*arbitrum_types.ConditionalOptions{ {}, {TimestampMin: &timeMin}, @@ -157,9 +156,9 @@ func optionsProduct(optionsA, optionsB []*arbitrum_types.ConditionalOptions) []* c.KnownAccounts[k] = v } limitTriples := []struct { - a *hexutil.Uint64 - b *hexutil.Uint64 - c **hexutil.Uint64 + a *common.Uint64OrHex + b *common.Uint64OrHex + c **common.Uint64OrHex }{ {a.BlockNumberMin, b.BlockNumberMin, &c.BlockNumberMin}, {a.BlockNumberMax, b.BlockNumberMax, &c.BlockNumberMax}, @@ -168,10 +167,10 @@ func optionsProduct(optionsA, optionsB []*arbitrum_types.ConditionalOptions) []* } for _, tripple := range limitTriples { if tripple.b != nil { - value := hexutil.Uint64(*tripple.b) + value := common.Uint64OrHex(*tripple.b) *tripple.c = &value } else if tripple.a != nil { - value := hexutil.Uint64(*tripple.a) + value := common.Uint64OrHex(*tripple.a) *tripple.c = &value } else { *tripple.c = nil From 1f6ddc4f16497f80a203fdfb8f7c4cca04bfba93 Mon Sep 17 00:00:00 2001 From: Nodar Date: Thu, 31 Aug 2023 17:55:40 +0200 Subject: [PATCH 16/22] Change warning to debug when decoding queued transaction fails, add another debug line when it succeeds --- arbnode/dataposter/storage/storage.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arbnode/dataposter/storage/storage.go b/arbnode/dataposter/storage/storage.go index ed848b9b7d..734e2770ee 100644 --- a/arbnode/dataposter/storage/storage.go +++ b/arbnode/dataposter/storage/storage.go @@ -103,11 +103,12 @@ func QueuedTransactionToLegacy(qt *QueuedTransaction) (*LegacyQueuedTransaction, func decode(data []byte) (*QueuedTransaction, error) { var item QueuedTransaction if err := rlp.DecodeBytes(data, &item); err != nil { - log.Warn("Failed to decode QueuedTransaction, attempting to decide legacy queued transaction", "error", err) + log.Debug("Failed to decode QueuedTransaction, attempting to decide legacy queued transaction", "error", err) val, err := DecodeLegacyQueuedTransaction(data) if err != nil { return nil, fmt.Errorf("decoding legacy item: %w", err) } + log.Debug("Succeeded decoding QueuedTransaction with legacy encoder") return LegacyToQueuedTransaction(val) } return &item, nil From 8f3f8162b88f22e3ef56e3ebbc6ba627b10b8ecb Mon Sep 17 00:00:00 2001 From: ganeshvanahalli Date: Thu, 31 Aug 2023 12:59:35 -0500 Subject: [PATCH 17/22] reuse type math.HexOrDecimal64 --- go-ethereum | 2 +- system_tests/conditionaltx_test.go | 29 +++++++++++++++-------------- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/go-ethereum b/go-ethereum index f8363dc42d..b4bd0da114 160000 --- a/go-ethereum +++ b/go-ethereum @@ -1 +1 @@ -Subproject commit f8363dc42d6cf8dbd76aed2550207a90919e88ee +Subproject commit b4bd0da1142fe6bb81cac7e0794ebb4746b9885a diff --git a/system_tests/conditionaltx_test.go b/system_tests/conditionaltx_test.go index b758ba1807..14aa000313 100644 --- a/system_tests/conditionaltx_test.go +++ b/system_tests/conditionaltx_test.go @@ -16,6 +16,7 @@ import ( "github.com/ethereum/go-ethereum/arbitrum" "github.com/ethereum/go-ethereum/arbitrum_types" "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/common/math" "github.com/ethereum/go-ethereum/core/types" "github.com/ethereum/go-ethereum/crypto" "github.com/ethereum/go-ethereum/rpc" @@ -102,23 +103,23 @@ func getOptions(address common.Address, rootHash common.Hash, slotValueMap map[c } func getFulfillableBlockTimeLimits(t *testing.T, blockNumber uint64, timestamp uint64) []*arbitrum_types.ConditionalOptions { - future := common.Uint64OrHex(timestamp + 30) - past := common.Uint64OrHex(timestamp - 1) - futureBlockNumber := common.Uint64OrHex(blockNumber + 1000) - currentBlockNumber := common.Uint64OrHex(blockNumber) + future := math.HexOrDecimal64(timestamp + 30) + past := math.HexOrDecimal64(timestamp - 1) + futureBlockNumber := math.HexOrDecimal64(blockNumber + 1000) + currentBlockNumber := math.HexOrDecimal64(blockNumber) return getBlockTimeLimits(t, currentBlockNumber, futureBlockNumber, past, future) } func getUnfulfillableBlockTimeLimits(t *testing.T, blockNumber uint64, timestamp uint64) []*arbitrum_types.ConditionalOptions { - future := common.Uint64OrHex(timestamp + 30) - past := common.Uint64OrHex(timestamp - 1) - futureBlockNumber := common.Uint64OrHex(blockNumber + 1000) - previousBlockNumber := common.Uint64OrHex(blockNumber - 1) + future := math.HexOrDecimal64(timestamp + 30) + past := math.HexOrDecimal64(timestamp - 1) + futureBlockNumber := math.HexOrDecimal64(blockNumber + 1000) + previousBlockNumber := math.HexOrDecimal64(blockNumber - 1) // skip first empty options return getBlockTimeLimits(t, futureBlockNumber, previousBlockNumber, future, past)[1:] } -func getBlockTimeLimits(t *testing.T, blockMin, blockMax common.Uint64OrHex, timeMin, timeMax common.Uint64OrHex) []*arbitrum_types.ConditionalOptions { +func getBlockTimeLimits(t *testing.T, blockMin, blockMax math.HexOrDecimal64, timeMin, timeMax math.HexOrDecimal64) []*arbitrum_types.ConditionalOptions { basic := []*arbitrum_types.ConditionalOptions{ {}, {TimestampMin: &timeMin}, @@ -156,9 +157,9 @@ func optionsProduct(optionsA, optionsB []*arbitrum_types.ConditionalOptions) []* c.KnownAccounts[k] = v } limitTriples := []struct { - a *common.Uint64OrHex - b *common.Uint64OrHex - c **common.Uint64OrHex + a *math.HexOrDecimal64 + b *math.HexOrDecimal64 + c **math.HexOrDecimal64 }{ {a.BlockNumberMin, b.BlockNumberMin, &c.BlockNumberMin}, {a.BlockNumberMax, b.BlockNumberMax, &c.BlockNumberMax}, @@ -167,10 +168,10 @@ func optionsProduct(optionsA, optionsB []*arbitrum_types.ConditionalOptions) []* } for _, tripple := range limitTriples { if tripple.b != nil { - value := common.Uint64OrHex(*tripple.b) + value := math.HexOrDecimal64(*tripple.b) *tripple.c = &value } else if tripple.a != nil { - value := common.Uint64OrHex(*tripple.a) + value := math.HexOrDecimal64(*tripple.a) *tripple.c = &value } else { *tripple.c = nil From d23602ffc8215367d7705c0e4867cb357949ba84 Mon Sep 17 00:00:00 2001 From: Lee Bousfield Date: Thu, 31 Aug 2023 19:42:15 -0600 Subject: [PATCH 18/22] Fix batch poster fixAccErr not resetting when the error stops --- arbnode/batch_poster.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/arbnode/batch_poster.go b/arbnode/batch_poster.go index 429701be7e..53cd5d5594 100644 --- a/arbnode/batch_poster.go +++ b/arbnode/batch_poster.go @@ -970,10 +970,14 @@ func (b *BatchPoster) Start(ctxIn context.Context) { return b.config().PollInterval } posted, err := b.maybePostSequencerBatch(ctx) + ephemeralError := errors.Is(err, AccumulatorNotFoundErr) || errors.Is(err, storage.ErrStorageRace) + if !ephemeralError { + b.firstAccErr = time.Time{} + } if err != nil { b.building = nil logLevel := log.Error - if errors.Is(err, AccumulatorNotFoundErr) || errors.Is(err, storage.ErrStorageRace) { + if ephemeralError { // Likely the inbox tracker just isn't caught up. // Let's see if this error disappears naturally. if b.firstAccErr == (time.Time{}) { @@ -982,8 +986,6 @@ func (b *BatchPoster) Start(ctxIn context.Context) { } else if time.Since(b.firstAccErr) < time.Minute { logLevel = log.Debug } - } else { - b.firstAccErr = time.Time{} } logLevel("error posting batch", "err", err) return b.config().ErrorDelay From 053e13a4d81da7e417be72c70f023bf86aa11189 Mon Sep 17 00:00:00 2001 From: Lee Bousfield Date: Thu, 31 Aug 2023 19:47:50 -0600 Subject: [PATCH 19/22] Rename firstAccErr to firstEphemeralError --- arbnode/batch_poster.go | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/arbnode/batch_poster.go b/arbnode/batch_poster.go index 53cd5d5594..43cf97f2ae 100644 --- a/arbnode/batch_poster.go +++ b/arbnode/batch_poster.go @@ -56,20 +56,20 @@ type batchPosterPosition struct { type BatchPoster struct { stopwaiter.StopWaiter - l1Reader *headerreader.HeaderReader - inbox *InboxTracker - streamer *TransactionStreamer - config BatchPosterConfigFetcher - seqInbox *bridgegen.SequencerInbox - bridge *bridgegen.Bridge - syncMonitor *SyncMonitor - seqInboxABI *abi.ABI - seqInboxAddr common.Address - building *buildingBatch - daWriter das.DataAvailabilityServiceWriter - dataPoster *dataposter.DataPoster - redisLock *redislock.Simple - firstAccErr time.Time // first time a continuous missing accumulator occurred + l1Reader *headerreader.HeaderReader + inbox *InboxTracker + streamer *TransactionStreamer + config BatchPosterConfigFetcher + seqInbox *bridgegen.SequencerInbox + bridge *bridgegen.Bridge + syncMonitor *SyncMonitor + seqInboxABI *abi.ABI + seqInboxAddr common.Address + building *buildingBatch + daWriter das.DataAvailabilityServiceWriter + dataPoster *dataposter.DataPoster + redisLock *redislock.Simple + firstEphemeralError time.Time // first time a continuous error suspected to be ephemeral occurred // An estimate of the number of batches we want to post but haven't yet. // This doesn't include batches which we don't want to post yet due to the L1 bounds. backlog uint64 @@ -972,7 +972,7 @@ func (b *BatchPoster) Start(ctxIn context.Context) { posted, err := b.maybePostSequencerBatch(ctx) ephemeralError := errors.Is(err, AccumulatorNotFoundErr) || errors.Is(err, storage.ErrStorageRace) if !ephemeralError { - b.firstAccErr = time.Time{} + b.firstEphemeralError = time.Time{} } if err != nil { b.building = nil @@ -980,10 +980,10 @@ func (b *BatchPoster) Start(ctxIn context.Context) { if ephemeralError { // Likely the inbox tracker just isn't caught up. // Let's see if this error disappears naturally. - if b.firstAccErr == (time.Time{}) { - b.firstAccErr = time.Now() + if b.firstEphemeralError == (time.Time{}) { + b.firstEphemeralError = time.Now() logLevel = log.Debug - } else if time.Since(b.firstAccErr) < time.Minute { + } else if time.Since(b.firstEphemeralError) < time.Minute { logLevel = log.Debug } } From 2f9db18cb552cec9647c9c6dfd9cbe4d7ef079b9 Mon Sep 17 00:00:00 2001 From: Nodar Date: Fri, 1 Sep 2023 16:34:51 +0200 Subject: [PATCH 20/22] Compare correct encodings in redis storage, drop nil rlp tag from queuedTransaction struct --- arbnode/dataposter/data_poster.go | 2 +- arbnode/dataposter/redis/redisstorage.go | 15 +++++++++++++++ arbnode/dataposter/storage/storage.go | 8 +++----- arbnode/dataposter/storage_test.go | 9 +++++++++ 4 files changed, 28 insertions(+), 6 deletions(-) diff --git a/arbnode/dataposter/data_poster.go b/arbnode/dataposter/data_poster.go index 5231a1c332..dff2602cac 100644 --- a/arbnode/dataposter/data_poster.go +++ b/arbnode/dataposter/data_poster.go @@ -112,7 +112,7 @@ func NewDataPoster(db ethdb.Database, headerReader *headerreader.HeaderReader, a case initConfig.UseNoOpStorage: queue = &noop.Storage{} case initConfig.UseLevelDB: - queue = leveldb.New(db, encF) + queue = leveldb.New(db, func() storage.EncoderDecoderInterface { return &storage.EncoderDecoder{} }) case redisClient == nil: queue = slice.NewStorage(func() storage.EncoderDecoderInterface { return &storage.EncoderDecoder{} }) default: diff --git a/arbnode/dataposter/redis/redisstorage.go b/arbnode/dataposter/redis/redisstorage.go index 5123cea154..f2393611b2 100644 --- a/arbnode/dataposter/redis/redisstorage.go +++ b/arbnode/dataposter/redis/redisstorage.go @@ -114,6 +114,17 @@ func (s *Storage) Prune(ctx context.Context, until uint64) error { return nil } +// normalizeDecoding decodes data (regardless of what encoding it used), and +// encodes it according to current encoding for storage. +// As a result, encoded data is transformed to currently used encoding. +func (s *Storage) normalizeDecoding(data []byte) ([]byte, error) { + item, err := s.encDec().Decode(data) + if err != nil { + return nil, err + } + return s.encDec().Encode(item) +} + func (s *Storage) Put(ctx context.Context, index uint64, prev, new *storage.QueuedTransaction) error { if new == nil { return fmt.Errorf("tried to insert nil item at index %v", index) @@ -142,6 +153,10 @@ func (s *Storage) Put(ctx context.Context, index uint64, prev, new *storage.Queu if err != nil { return fmt.Errorf("failed to validate item already in redis at index%v: %w", index, err) } + verifiedItem, err = s.normalizeDecoding(verifiedItem) + if err != nil { + return fmt.Errorf("error normalizing encoding for verified item: %w", err) + } prevItemEncoded, err := s.encDec().Encode(prev) if err != nil { return err diff --git a/arbnode/dataposter/storage/storage.go b/arbnode/dataposter/storage/storage.go index 734e2770ee..b59bf7bf62 100644 --- a/arbnode/dataposter/storage/storage.go +++ b/arbnode/dataposter/storage/storage.go @@ -21,7 +21,7 @@ var ( ) type QueuedTransaction struct { - FullTx *types.Transaction `rlp:"nil"` + FullTx *types.Transaction Data types.DynamicFeeTx Meta []byte Sent bool @@ -35,7 +35,7 @@ type QueuedTransaction struct { // rlp encoding of Meta into queuedTransaction and rlp encoding it once more // to store it. type LegacyQueuedTransaction struct { - FullTx *types.Transaction `rlp:"nil"` + FullTx *types.Transaction Data types.DynamicFeeTx Meta BatchPosterPosition Sent bool @@ -46,9 +46,7 @@ type LegacyQueuedTransaction struct { // This is also for legacy reason. Since Batchposter is in arbnode package, // we can't refer to BatchPosterPosition type there even if we export it (that // would create cyclic dependency). -// Ideally we'll factor out Batch Poster from arbnode into separate package -// and BatchPosterPosition into another separate package as well. -// For the sake of minimal refactoring, that struct is duplicated here. +// We'll drop this struct in a few releases when we drop legacy encoding. type BatchPosterPosition struct { MessageCount arbutil.MessageIndex DelayedMessageCount uint64 diff --git a/arbnode/dataposter/storage_test.go b/arbnode/dataposter/storage_test.go index eac05502be..d536e5da05 100644 --- a/arbnode/dataposter/storage_test.go +++ b/arbnode/dataposter/storage_test.go @@ -6,6 +6,7 @@ import ( "path" "testing" + "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/core/rawdb" "github.com/ethereum/go-ethereum/core/types" "github.com/ethereum/go-ethereum/rlp" @@ -22,6 +23,7 @@ import ( var ignoreData = cmp.Options{ cmpopts.IgnoreUnexported( + types.Transaction{}, types.DynamicFeeTx{}, big.Int{}, ), @@ -62,6 +64,13 @@ func valueOf(t *testing.T, i int) *storage.QueuedTransaction { t.Fatalf("Encoding batch poster position, error: %v", err) } return &storage.QueuedTransaction{ + FullTx: types.NewTransaction( + uint64(i), + common.Address{}, + big.NewInt(int64(i)), + uint64(i), + big.NewInt(int64(i)), + []byte{byte(i)}), Meta: meta, Data: types.DynamicFeeTx{ ChainID: big.NewInt(int64(i)), From 0bf724b80497f821ff31fc36519ffe53b37d9676 Mon Sep 17 00:00:00 2001 From: Nodar Date: Fri, 1 Sep 2023 17:09:28 +0200 Subject: [PATCH 21/22] Fix incorrect merge with base pr --- linter/koanf/koanf.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/linter/koanf/koanf.go b/linter/koanf/koanf.go index d483cf7751..c7c38e2571 100644 --- a/linter/koanf/koanf.go +++ b/linter/koanf/koanf.go @@ -116,7 +116,7 @@ func checkFlagDefs(pass *analysis.Pass, f *ast.FuncDecl) Result { if !ok { continue } - if normSL := normalize(sl); !strings.EqualFold(normSL, s) { + if normSL := strings.ReplaceAll(sl, "-", ""); !strings.EqualFold(normSL, s) { res.Errors = append(res.Errors, koanfError{ Pos: pass.Fset.Position(f.Pos()), Message: fmt.Sprintf("koanf tag name: %q doesn't match the field: %q", sl, s), From 1182da1bc3fef5f9117edb64323f0480b6e1f63b Mon Sep 17 00:00:00 2001 From: Lee Bousfield Date: Fri, 1 Sep 2023 09:54:04 -0600 Subject: [PATCH 22/22] Fix changing the basefee in non-mutating calls --- arbos/tx_processor.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arbos/tx_processor.go b/arbos/tx_processor.go index 09a4692eae..0d44ac548e 100644 --- a/arbos/tx_processor.go +++ b/arbos/tx_processor.go @@ -677,7 +677,7 @@ func (p *TxProcessor) GetPaidGasPrice() *big.Int { if version != 9 { gasPrice = p.evm.Context.BaseFee if p.msg.TxRunMode != core.MessageCommitMode && p.msg.GasFeeCap.Sign() == 0 { - gasPrice.SetInt64(0) // gasprice zero behavior + gasPrice = common.Big0 } } return gasPrice