Add more functionality to the sim #5

gitteri · 2019-10-21T12:40:56Z

Adds many new functions for the eth sim and tests.

PD added TransactionByHashWithBlockNum. I'll look to see if we need this and if we should keep it in this pr. Goal is to upstream this to go-etherem. Note: this is up to date with go-ethereum's current master

…r client

Fixing the Simulated and Adding a new Fn TransactionByHashWithBlockNum

gitteri · 2019-10-21T12:44:47Z

ethclient/ethclient.go

+// TransactionByHash returns the transaction with the given hash.
+// The code is exactly copied from the function TransactionByHash
+// but this returns the blockHexString instead of json.BlockNumber == nil
+func (ec *Client) TransactionByHashWithBlockNum(ctx context.Context, hash common.Hash) (tx *types.Transaction, blockHexString string, err error) {


Not sure if we need this. I'll look through for usage and also update the comment to reflect what it actually does if we do

Hmm.. ya if this was in a public PR I think it might open a separate can of worms. For Receipts we added block info to the Receipt struct and made changes throughout geth to populate it. For a Transaction there is already an RPC transaction, which may be worth migration to or returning here.

Yeah I would anticipate resistance to the TransactionReader interface change - adding interface methods is a breaking change for other implementors.

Potentially-silly question, not knowing the contents of the return structs: A transaction won't be assigned a block num unless it has been mined, right? So we ought to be able to use a combination of the newTransactionReceipt (with block info) and TransactionByHashto accomplish whatever we were doing with this method?

Yeah I pushed back pretty hard when he was trying to add this but I guess he just did it anyway

gitteri · 2019-10-21T12:45:55Z

accounts/abi/bind/backends/simulated.go

@@ -419,10 +541,38 @@ func (b *SimulatedBackend) SubscribeFilterLogs(ctx context.Context, query ethere
 	}), nil
 }

+// SubscribeNewHead returns an event subscription for a new header
+func (b *SimulatedBackend) SubscribeNewHead(ctx context.Context, ch chan<- *types.Header) (ethereum.Subscription, error) {


Needed this functionality implement the ChainReader interface

bmperrea · 2019-10-21T13:15:00Z

accounts/abi/bind/backends/simulated.go

+
+	tx, _, blockNumber, _ := rawdb.ReadTransaction(b.database, txHash)
+	if tx != nil {
+		return tx, hexutil.EncodeUint64(blockNumber), nil


why not return uint64 here?

hopefully we can just remove this function

bmperrea · 2019-10-21T13:16:08Z

accounts/abi/bind/backends/simulated.go

+		return nil, errBlockDoesNotExist
+	}
+
+	return block.Transactions()[index], nil


is there a scenario where block.Transactions returns nil?

doesn't look like it returns nil but it could return a nil slice. adding a check

do we still want to add a check here? I see another case of block.Transactions()[index] a few lines above.

improved this function a bit and added some of the error cases to the test

bmperrea · 2019-10-21T13:18:33Z

ethclient/ethclient.go

+// TransactionByHash returns the transaction with the given hash.
+// The code is exactly copied from the function TransactionByHash
+// but this returns the blockHexString instead of json.BlockNumber == nil
+func (ec *Client) TransactionByHashWithBlockNum(ctx context.Context, hash common.Hash) (tx *types.Transaction, blockHexString string, err error) {


Hmm.. ya if this was in a public PR I think it might open a separate can of worms. For Receipts we added block info to the Receipt struct and made changes throughout geth to populate it. For a Transaction there is already an RPC transaction, which may be worth migration to or returning here.

jgiles · 2019-10-21T13:56:02Z

accounts/abi/bind/backends/simulated.go

+// SubscribeNewHead returns an event subscription for a new header
+func (b *SimulatedBackend) SubscribeNewHead(ctx context.Context, ch chan<- *types.Header) (ethereum.Subscription, error) {
+	// subscribe to a new head
+	sink := make(chan *types.Header)


nit: unless this is an established naming convention in this kind of method, maybe upstream ? It's a little weird to call it "sink" in the context of this function because we do not write to the it.

Yeah I found sink elsewhere when dealing with subscriptions:
Subscribe(sink chan<- WalletEvent) event.Subscription

jgiles · 2019-10-21T14:31:28Z

accounts/abi/bind/backends/simulated.go

+			select {
+			case head := <-sink:
+				select {
+				case ch <- head:


we have a nested select because the send to ch might block, and we want to be able to handle the error or quit messages while it's blocked?

It's probably simpler to avoid the nesting:

waitWrite := make(chan *types.Header) defer close(curr) var source <-chan *types.Header = upstream for { var curr *types.Header select { case head := <-upstream: curr = head case err ... case quit... } select { case ch <- curr: continue case err... case quit... } }

(unless the nesting is an established pattern in go-ethereum)

Yeah I see nested selects everywhere:

node/api.go line 128 contract/oracle line 379 eth/enr_entry.go line 45 miner/miner.go line 89 p2p/simulations/network.go line 977

and many more

connorwstein · 2019-10-24T15:18:40Z

accounts/abi/bind/backends/simulated_test.go

+		t.Errorf("could not estimate gas: %v", err)
+	}
+
+	if gas != uint64(21000) {


I'd just use params.TxGas rather than hard code

true throughout these tests

connorwstein · 2019-10-24T15:32:33Z

accounts/abi/bind/backends/simulated_test.go

+	if len(res) == 0 {
+		t.Errorf("result of contract call was empty: %v", res)
+	}
+	// use strings contains since actual response is padded with many \x00 bytes and a space


The padding shouldn't change though right? If so we could look for the exact bytes to tighten up this test

* backends: implement more of ethclient in sim * backends: add BlockByNumber to simulated backend * backends: make simulated progress function agree with syncprogress interface for client * backends: add more tests * backends: add more comments * backends: fix sim for index in tx and add tests

* eth/protocols: persist received state segments * core: initial implementation * core/state/snapshot: add tests * core, eth: updates * eth/protocols/snapshot: count flat state size * core/state: add metrics * core/state/snapshot: skip unnecessary deletion * core/state/snapshot: rename * core/state/snapshot: use the global batch * core/state/snapshot: add logs and fix wiping * core/state/snapshot: fix * core/state/snapshot: save generation progress even if the batch is empty * core/state/snapshot: fixes * core/state/snapshot: fix initial account range length * core/state/snapshot: fix initial account range * eth/protocols/snap: store flat states during the healing * eth/protocols/snap: print logs * core/state/snapshot: refactor (#4) * core/state/snapshot: refactor * core/state/snapshot: tiny fix and polish Co-authored-by: rjl493456442 <garyrong0905@gmail.com> * core, eth: fixes * core, eth: fix healing writer * core, trie, eth: fix paths * eth/protocols/snap: fix encoding * eth, core: add debug log * core/state/generate: release iterator asap (#5) core/state/snapshot: less copy core/state/snapshot: revert split loop core/state/snapshot: handle storage becoming empty, improve test robustness core/state: test modified codehash core/state/snapshot: polish * core/state/snapshot: optimize stats counter * core, eth: add metric * core/state/snapshot: update comments * core/state/snapshot: improve tests * core/state/snapshot: replace secure trie with standard trie * core/state/snapshot: wrap return as the struct * core/state/snapshot: skip wiping correct states * core/state/snapshot: updates * core/state/snapshot: fixes * core/state/snapshot: fix panic due to reference flaw in closure * core/state/snapshot: fix errors in state generation logic + fix log output * core/state/snapshot: remove an error case * core/state/snapshot: fix condition-check for exhausted snap state * core/state/snapshot: use stackTrie for small tries * core/state/snapshot: don't resolve small storage tries in vain * core/state/snapshot: properly clean up storage of deleted accounts * core/state/snapshot: avoid RLP-encoding in some cases + minor nitpicks * core/state/snapshot: fix error (+testcase) * core/state/snapshot: clean up tests a bit * core/state/snapshot: work in progress on better tests * core/state/snapshot: polish code * core/state/snapshot: fix trie iteration abortion trigger * core/state/snapshot: fixes flaws * core/state/snapshot: remove panic * core/state/snapshot: fix abort * core/state/snapshot: more tests (plus failing testcase) * core/state/snapshot: more testcases + fix for failing test * core/state/snapshot: testcase for malformed data * core/state/snapshot: some test nitpicks * core/state/snapshot: improvements to logging * core/state/snapshot: testcase to demo error in abortion * core/state/snapshot: fix abortion * cmd/geth: make verify-state report the root * trie: fix failing test * core/state/snapshot: add timer metrics * core/state/snapshot: fix metrics * core/state/snapshot: udpate tests * eth/protocols/snap: write snapshot account even if code or state is needed * core/state/snapshot: fix diskmore check * core/state/snapshot: review fixes * core/state/snapshot: improve error message * cmd/geth: rename 'error' to 'err' in logs * core/state/snapshot: fix some review concerns * core/state/snapshot, eth/protocols/snap: clear snapshot marker when starting/resuming snap sync * core: add error log * core/state/snapshot: use proper timers for metrics collection * core/state/snapshot: address some review concerns * eth/protocols/snap: improved log message * eth/protocols/snap: fix heal logs to condense infos * core/state/snapshot: wait for generator termination before restarting * core/state/snapshot: revert timers to counters to track total time Co-authored-by: Martin Holst Swende <martin@swende.se> Co-authored-by: Péter Szilágyi <peterke@gmail.com>

bmperrea and others added 30 commits October 18, 2018 18:54

types, rawdb: add block location fields to receipt (#2)

912cc46

backends: implement more of ethclient in sim

e7dc018

add BlockByNumber to simulated backend

47d1a8a

add Progress to get current blockheight

bc59e80

make simulated progress function agree with syncprogress interface fo…

23c1efe

…r client

update function comment to match new function name

e6f04b6

allow for nil block number to get most recent block

2e8ecf3

update getblockbynumber to use same type casting as headerbynumber

de06789

add interface compliance to sim

6da3643

Fixing the Simulated and Adding a new Fn TransactionByHashWithBlockNum

020746b

Fixing the Simulated and Adding a new Fn TransactionByHashWithBlockNum

3575e99

Fixing the Simulated and Adding a new Fn TransactionByHashWithBlockNum

f6444de

Fixing the Simulated and Adding a new Fn TransactionByHashWithBlockNum

72a8d02

Fixing the Simulated and Adding a new Fn TransactionByHashWithBlockNum

596b253

Fixing the Simulated and Adding a new Fn TransactionByHashWithBlockNum

64da30a

Fixing the Simulated and Adding a new Fn TransactionByHashWithBlockNum

707936b

Fixing the Simulated and Adding a new Fn TransactionByHashWithBlockNum

f39b8ec

Fixing the Simulated and Adding a new Fn TransactionByHashWithBlockNum

8f4905b

Merge pull request #2 from paxosglobal/fix-transactionbyhash

817dd6c

Fixing the Simulated and Adding a new Fn TransactionByHashWithBlockNum

Fixing the Simulated and Adding a new Fn TransactionByHashWithBlockNum

d771c3f

Merge pull request #3 from paxosglobal/fix-transactionbyhash

e139f6c

Fixing the Simulated and Adding a new Fn TransactionByHashWithBlockNum

Fixing the Simulated and Adding a new Fn TransactionByHashWithBlockNum

1338611

Merge pull request #4 from paxosglobal/fix-transactionbyhash

66bfe0e

Fixing the Simulated and Adding a new Fn TransactionByHashWithBlockNum

update with go-ethereum master

2b52d8e

update vendor and add more sim tests

abc197f

update functions and add more tests

eee0ff9

add more comments and improve features for current block

ceb4643

Merge branch 'master' into igitter/sim-tested

a4381e5

reset vendor to master

31ac697

remove mod files from vendor

5f6df0f

gitteri commented Oct 21, 2019

View reviewed changes

bmperrea reviewed Oct 21, 2019

View reviewed changes

jgiles reviewed Oct 21, 2019

View reviewed changes

remove unused methods and clean up variables

0ff0e57

connorwstein approved these changes Oct 24, 2019

View reviewed changes

ilan gitter added 2 commits October 24, 2019 16:09

Merge branch 'master' into igitter/sim-tested

7215e10

fix sim for index in tx and add tests

65fd773

gitteri changed the base branch from master to igitter-sim-improved October 26, 2019 14:23

gitteri merged commit ed6b7de into igitter-sim-improved Oct 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more functionality to the sim #5

Add more functionality to the sim #5

gitteri commented Oct 21, 2019

gitteri Oct 21, 2019

bmperrea Oct 21, 2019

jgiles Oct 21, 2019

gitteri Oct 21, 2019

gitteri Oct 24, 2019

gitteri Oct 21, 2019

bmperrea Oct 21, 2019

gitteri Oct 21, 2019

bmperrea Oct 21, 2019

gitteri Oct 21, 2019

bmperrea Oct 24, 2019

gitteri Oct 24, 2019

bmperrea Oct 21, 2019

jgiles Oct 21, 2019

gitteri Oct 24, 2019

jgiles Oct 21, 2019

gitteri Oct 24, 2019

gitteri Oct 24, 2019

connorwstein Oct 24, 2019

connorwstein Oct 24, 2019

gitteri Oct 24, 2019

connorwstein Oct 24, 2019

gitteri Oct 24, 2019

Add more functionality to the sim #5

Add more functionality to the sim #5

Conversation

gitteri commented Oct 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment