Merge branch 'develop' into aj/revert-retry-client

op-chain-ops/genesis/layer_one.go

@@ -244,6 +244,11 @@ func BuildL1DeveloperGenesis(config *DeployConfig) (*core.Genesis, error) {
 		return nil, err
 	}
 
+	if config.FundDevAccounts {
+		FundDevAccounts(stateDB)
+		SetPrecompileBalances(stateDB)
+	}
+
 	for _, dep := range deployments {
 		st, err := stateDB.StorageTrie(dep.Address)
 		if err != nil {

op-challenger/fault/cmd/main.go

@@ -18,10 +18,10 @@ func main() {
 	p.Attack()
 	p.Print(3)
 
-	// Trace Position is 0000	Trace Depth is: 0	Trace Index is: 8
-	// Trace Position is 0000	Trace Depth is: 1	Trace Index is: 4
-	// Trace Position is 0010	Trace Depth is: 2	Trace Index is: 6
-	// Trace Position is 0100	Trace Depth is: 3	Trace Index is: 5
+	// GIN:    1	Trace Position is    0	Trace Depth is: 0	Trace Index is: 8
+	// GIN:   10	Trace Position is    0	Trace Depth is: 1	Trace Index is: 4
+	// GIN:  110	Trace Position is   10	Trace Depth is: 2	Trace Index is: 6
+	// GIN: 1100	Trace Position is  100	Trace Depth is: 3	Trace Index is: 5
 
 	// Example 2
 	// abcdefgh

op-challenger/fault/position.go

@@ -3,36 +3,42 @@ package fault
 import "fmt"
 
 // Position is a golang wrapper around the dispute game Position type.
-// Depth refers to how many bisection steps have occurred.
-// IndexAtDepth refers to the path that the bisection has taken
-// where 1 = goes right & 0 = goes left.
 type Position struct {
-	Depth        int
-	IndexAtDepth int
+	depth        int
+	indexAtDepth int
 }
 
-// TraceIndex calculates the what the index of the claim value
-// would be inside the trace.
+func NewPosition(depth, indexAtDepth int) Position {
+	return Position{depth, indexAtDepth}
+}
+
+func NewPositionFromGIndex(x uint64) Position {
+	depth := MSBIndex(x)
+	indexAtDepth := ^(1 << depth) & x
+	return NewPosition(depth, int(indexAtDepth))
+}
+
+func (p *Position) Depth() int {
+	return p.depth
+}
+
+func (p *Position) IndexAtDepth() int {
+	return p.indexAtDepth
+}
+
+// TraceIndex calculates the what the index of the claim value would be inside the trace.
+// It is equivalent to going right until the final depth has been reached.
 func (p *Position) TraceIndex(maxDepth int) int {
-	lo := 0
-	hi := 1 << maxDepth
-	mid := hi
-	path := p.IndexAtDepth
-	for i := p.Depth - 1; i >= 0; i-- {
-		mid = (lo + hi) / 2
-		mask := 1 << i
-		if path&mask == mask {
-			lo = mid
-		} else {
-			hi = mid
-		}
-	}
-	return mid
+	// When we go right, we do a shift left and set the bottom bit to be 1.
+	// To do this in a single step, do all the shifts at once & or in all 1s for the bottom bits.
+	rd := maxDepth - p.depth
+	return p.indexAtDepth<<rd | ((1 << rd) - 1)
 }
 
+// move goes to the left or right child.
 func (p *Position) move(right bool) {
-	p.Depth++
-	p.IndexAtDepth = (p.IndexAtDepth << 1) | boolToInt(right)
+	p.depth++
+	p.indexAtDepth = (p.indexAtDepth << 1) | boolToInt(right)
 }
 
 func boolToInt(b bool) int {
@@ -43,15 +49,18 @@ func boolToInt(b bool) int {
 	}
 }
 
+// parent moves up to the parent.
 func (p *Position) parent() {
-	p.Depth--
-	p.IndexAtDepth = p.IndexAtDepth >> 1
+	p.depth--
+	p.indexAtDepth = p.indexAtDepth >> 1
 }
 
+// Attack moves this position to a position to the left which disagrees with this position.
 func (p *Position) Attack() {
 	p.move(false)
 }
 
+// Defend moves this position to the right which agrees with this position. Note:
 func (p *Position) Defend() {
 	p.parent()
 	p.move(true)
@@ -59,5 +68,21 @@ func (p *Position) Defend() {
 }
 
 func (p *Position) Print(maxDepth int) {
-	fmt.Printf("Trace Position is %04b\tTrace Depth is: %d\tTrace Index is: %d\n", p.IndexAtDepth, p.Depth, p.TraceIndex(maxDepth))
+	fmt.Printf("GIN: %4b\tTrace Position is %4b\tTrace Depth is: %d\tTrace Index is: %d\n", p.ToGIndex(), p.indexAtDepth, p.depth, p.TraceIndex(maxDepth))
+}
+
+func (p *Position) ToGIndex() uint64 {
+	return uint64(1<<p.depth | p.indexAtDepth)
+}
+
+// MSBIndex returns the index of the most significant bit
+func MSBIndex(x uint64) int {
+	if x == 0 {
+		return 0
+	}
+	out := 0
+	for ; x != 0; out++ {
+		x = x >> 1
+	}
+	return out - 1
 }

op-challenger/fault/position_test.go

 package fault
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestMSBIndex(t *testing.T) {
+	tests := []struct {
+		input    uint64
+		expected int
+	}{
+		{0, 0},
+		{1, 0},
+		{2, 1},
+		{4, 2},
+		{8, 3},
+		{16, 4},
+		{255, 7},
+		{1024, 10},
+		{18446744073709551615, 63},
+	}
+
+	for _, test := range tests {
+		result := MSBIndex(test.input)
+		if result != test.expected {
+			t.Errorf("MSBIndex(%d) expected %d, but got %d", test.input, test.expected, result)
+		}
+	}
+
+}
+
+type testNodeInfo struct {
+	GIndex       uint64
+	Depth        int
+	IndexAtDepth int
+	TraceIndex   int
+}
+
+var treeNodesMaxDepth4 = []testNodeInfo{
+	{GIndex: 1, Depth: 0, IndexAtDepth: 0, TraceIndex: 15},
+
+	{GIndex: 2, Depth: 1, IndexAtDepth: 0, TraceIndex: 7},
+	{GIndex: 3, Depth: 1, IndexAtDepth: 1, TraceIndex: 15},
+
+	{GIndex: 4, Depth: 2, IndexAtDepth: 0, TraceIndex: 3},
+	{GIndex: 5, Depth: 2, IndexAtDepth: 1, TraceIndex: 7},
+	{GIndex: 6, Depth: 2, IndexAtDepth: 2, TraceIndex: 11},
+	{GIndex: 7, Depth: 2, IndexAtDepth: 3, TraceIndex: 15},
+
+	{GIndex: 8, Depth: 3, IndexAtDepth: 0, TraceIndex: 1},
+	{GIndex: 9, Depth: 3, IndexAtDepth: 1, TraceIndex: 3},
+	{GIndex: 10, Depth: 3, IndexAtDepth: 2, TraceIndex: 5},
+	{GIndex: 11, Depth: 3, IndexAtDepth: 3, TraceIndex: 7},
+	{GIndex: 12, Depth: 3, IndexAtDepth: 4, TraceIndex: 9},
+	{GIndex: 13, Depth: 3, IndexAtDepth: 5, TraceIndex: 11},
+	{GIndex: 14, Depth: 3, IndexAtDepth: 6, TraceIndex: 13},
+	{GIndex: 15, Depth: 3, IndexAtDepth: 7, TraceIndex: 15},
+
+	{GIndex: 16, Depth: 4, IndexAtDepth: 0, TraceIndex: 0},
+	{GIndex: 17, Depth: 4, IndexAtDepth: 1, TraceIndex: 1},
+	{GIndex: 18, Depth: 4, IndexAtDepth: 2, TraceIndex: 2},
+	{GIndex: 19, Depth: 4, IndexAtDepth: 3, TraceIndex: 3},
+	{GIndex: 20, Depth: 4, IndexAtDepth: 4, TraceIndex: 4},
+	{GIndex: 21, Depth: 4, IndexAtDepth: 5, TraceIndex: 5},
+	{GIndex: 22, Depth: 4, IndexAtDepth: 6, TraceIndex: 6},
+	{GIndex: 23, Depth: 4, IndexAtDepth: 7, TraceIndex: 7},
+	{GIndex: 24, Depth: 4, IndexAtDepth: 8, TraceIndex: 8},
+	{GIndex: 25, Depth: 4, IndexAtDepth: 9, TraceIndex: 9},
+	{GIndex: 26, Depth: 4, IndexAtDepth: 10, TraceIndex: 10},
+	{GIndex: 27, Depth: 4, IndexAtDepth: 11, TraceIndex: 11},
+	{GIndex: 28, Depth: 4, IndexAtDepth: 12, TraceIndex: 12},
+	{GIndex: 29, Depth: 4, IndexAtDepth: 13, TraceIndex: 13},
+	{GIndex: 30, Depth: 4, IndexAtDepth: 14, TraceIndex: 14},
+	{GIndex: 31, Depth: 4, IndexAtDepth: 15, TraceIndex: 15},
+}
+
+// TestGINConversions does To & From the generalized index on the treeNodesMaxDepth4 data
+func TestGINConversions(t *testing.T) {
+	for _, test := range treeNodesMaxDepth4 {
+		from := NewPositionFromGIndex(test.GIndex)
+		pos := NewPosition(test.Depth, test.IndexAtDepth)
+		require.Equal(t, pos, from)
+		to := pos.ToGIndex()
+		require.Equal(t, test.GIndex, to)
+	}
+}
+
+// TestTraceIndex creates the position & then tests the trace index function on the treeNodesMaxDepth4 data
+func TestTraceIndex(t *testing.T) {
+	for _, test := range treeNodesMaxDepth4 {
+		pos := NewPosition(test.Depth, test.IndexAtDepth)
+		result := pos.TraceIndex(4)
+		require.Equal(t, test.TraceIndex, result)
+	}
+}

op-node/p2p/sync.go

@@ -268,11 +268,11 @@ func (s *SyncClient) Start() {
 func (s *SyncClient) AddPeer(id peer.ID) {
 	s.peersLock.Lock()
 	defer s.peersLock.Unlock()
-	if _, ok := s.peers[id]; ok {
-		s.log.Warn("cannot register peer for sync duties, peer was already registered", "peer", id)
+	if s.closingPeers {
 		return
 	}
-	if s.closingPeers {
+	if _, ok := s.peers[id]; ok {
+		s.log.Warn("cannot register peer for sync duties, peer was already registered", "peer", id)
 		return
 	}
 	s.wg.Add(1)
@@ -492,9 +492,9 @@ func (s *SyncClient) peerLoop(ctx context.Context, id peer.ID) {
 	defer func() {
 		s.peersLock.Lock()
 		delete(s.peers, id) // clean up
+		s.log.Debug("stopped syncing loop of peer", "id", id)
 		s.wg.Done()
 		s.peersLock.Unlock()
-		s.log.Debug("stopped syncing loop of peer", "id", id)
 	}()
 
 	log := s.log.New("peer", id)

packages/contracts-bedrock/STYLE_GUIDE.md

@@ -10,8 +10,8 @@ with priority on the `L2OutputOracle` and `OptimismPortal`.
 
 #### Comments
 
-We use [Seaport](https://github.com/ProjectOpenSea/seaport/blob/main/contracts/Seaport.sol)-style comments with some minor modifications.
-Some basic rules:
+Optimism smart contracts follow the triple-slash [solidity natspec comment style](https://docs.soliditylang.org/en/develop/natspec-format.html#documentation-example)
+with additional rules. These are:
 
 - Always use `@notice` since it has the same general effect as `@dev` but avoids confusion about when to use one over the other.
 - Include a newline between `@notice` and the first `@param`.
@@ -28,13 +28,19 @@ We also have the following custom tags:
 #### Errors
 
 - Use `require` statements when making simple assertions.
-- Use `revert` if throwing an error where an assertion is not being made (no custom errors). See [here](https://github.com/ethereum-optimism/optimism/blob/861ae315a6db698a8c0adb1f8eab8311fd96be4c/packages/contracts-bedrock/contracts/L2/OVM_ETH.sol#L31) for an example of this in practice.
+- Use `revert(string)` if throwing an error where an assertion is not being made (no custom errors).
+  See [here](https://github.com/ethereum-optimism/optimism/blob/861ae315a6db698a8c0adb1f8eab8311fd96be4c/packages/contracts-bedrock/contracts/L2/OVM_ETH.sol#L31)
+  for an example of this in practice.
 - Error strings MUST have the format `"{ContractName}: {message}"` where `message` is a lower case string.
 
 #### Function Parameters
 
 - Function parameters should be prefixed with an underscore.
 
+#### Function Return Arguments
+
+- Arguments returned by functions should be suffixed with an underscore.
+
 #### Event Parameters
 
 - Event parameters should NOT be prefixed with an underscore.
@@ -42,16 +48,21 @@ We also have the following custom tags:
 #### Spacers
 
 We use spacer variables to account for old storage slots that are no longer being used.
-The name of a spacer variable MUST be in the format `spacer_<slot>_<offset>_<length>` where `<slot>` is the original storage slot number, `<offset>` is the original offset position within the storage slot, and `<length>` is the original size of the variable.
+The name of a spacer variable MUST be in the format `spacer_<slot>_<offset>_<length>` where
+`<slot>` is the original storage slot number, `<offset>` is the original offset position
+within the storage slot, and `<length>` is the original size of the variable.
 Spacers MUST be `private`.
 
 ### Proxy by Default
 
 All contracts should be assumed to live behind proxies (except in certain special circumstances).
 This means that new contracts MUST be built under the assumption of upgradeability.
-We use a minimal [`Proxy`](./contracts/universal/Proxy.sol) contract designed to be owned by a corresponding [`ProxyAdmin`](./contracts/universal/ProxyAdmin.sol) which follow the interfaces of OpenZeppelin's `Proxy` and `ProxyAdmin` contracts, respectively.
+We use a minimal [`Proxy`](./contracts/universal/Proxy.sol) contract designed to be owned by a
+corresponding [`ProxyAdmin`](./contracts/universal/ProxyAdmin.sol) which follow the interfaces
+of OpenZeppelin's `Proxy` and `ProxyAdmin` contracts, respectively.
 
-Unless explicitly discussed otherwise, you MUST include the following basic upgradeability pattern for each new implementation contract:
+Unless explicitly discussed otherwise, you MUST include the following basic upgradeability
+pattern for each new implementation contract:
 
 1. Extend OpenZeppelin's `Initializable` base contract.
 2. Include a `uint8 public constant VERSION = X` at the TOP of your contract.
@@ -60,11 +71,13 @@ Unless explicitly discussed otherwise, you MUST include the following basic upgr
 
 ### Versioning
 
-All (non-library and non-abstract) contracts MUST extend the `Semver` base contract which exposes a `version()` function that returns a semver-compliant version string.
-During the Bedrock development process the `Semver` value for all contracts SHOULD return `0.0.1` (this is not particularly important, but it's an easy standard to follow).
-When the initial Bedrock upgrade is released, the `Semver` value MUST be updated to `1.0.0`.
+All (non-library and non-abstract) contracts MUST extend the `Semver` base contract which
+exposes a `version()` function that returns a semver-compliant version string.
+
+Contracts must have a `Semver` of `1.0.0` or greater to be production ready. Contracts
+with `Semver` values less than `1.0.0` should only be used locally or on devnets.
 
-After the initial Bedrock upgrade, contracts MUST use the following versioning scheme:
+Additionally, contracts MUST use the following versioning scheme:
 
 - `patch` releases are to be used only for changes that do NOT modify contract bytecode (such as updating comments).
 - `minor` releases are to be used for changes that modify bytecode OR changes that expand the contract ABI provided that these changes do NOT break the existing interface.
@@ -72,7 +85,8 @@ After the initial Bedrock upgrade, contracts MUST use the following versioning s
 
 #### Exceptions
 
-We have made an exception to the `Semver` rule for the `WETH` contract to avoid making changes to a well-known, simple, and recognizable contract.
+We have made an exception to the `Semver` rule for the `WETH` contract to avoid
+making changes to a well-known, simple, and recognizable contract.
 
 ### Dependencies