Fix for issue #146 #158

i-norden · 2019-10-10T18:53:46Z

Mark header checked for contract if it doesn't have any logs at that header but other contracts do.

Test added.

rmulhol

Thanks for putting in work on this! Dig all the linter fixes 👍

Wondering if we could potentially simplify things by just marking all header fields checked after we iterated through all the logs we got back from the fetcher.

rmulhol · 2019-10-25T16:37:59Z

pkg/contract_watcher/header/transformer/transformer.go

@@ -126,6 +128,7 @@ func (tr *Transformer) Init() error {
 		firstBlock, retrieveErr := tr.Retriever.RetrieveFirstBlock()
 		if retrieveErr != nil {
 			if retrieveErr == sql.ErrNoRows {
+				logrus.Error(retrieveErr)


👍

Maybe worth adding some context with an Errorf?

Good point, will do!

rmulhol · 2019-10-25T16:45:11Z

pkg/contract_watcher/header/transformer/transformer.go

@@ -249,19 +253,32 @@ func (tr *Transformer) Execute() error {
 		}


What if just moved all the stuff in this guard to the bottom of the loop that's iterating through missing headers? Seems like the below loops through allLogs and sortedLogs would be a no-op and we could delete some code

A bit frazzled at the moment but I think I see what you mean and if I'm understanding correctly then the only thing that gives me pause is what to do if we run into an error for a header. We still want to mark the headers processed up until that point as checked which is why I was marking them as we go, but the much better approach would be to keep track of which index in the headers we are at when the error occurs and mark all those below it as checked in a single batch call. Looks like I've introduced quite a bit of unnecessary complexity here, thanks for catching that!

Yeah I mean my thought was just that we'd call MarkHeaderChecked once per header, after we've iterated through all of its logs. [not proposing that we try to mark multiple headers checked at once, though I'm interested to check it out if it seems preferable]

I guess the likely objection would be making sure we preserve the ability to handle new contracts/logs being watched later on, but I think that would work fine? Assuming that a new checked column gets added and defaults to false...

So after taking a closer look again, the issue there is that the header is normally checked marked in the same transaction that is used to persist logs in this call. So what we have currently are calls to mark the header checked in the cases where that call is never reached.

It's a quick change though, and one that will remove some excess code- making it now.

any logs at that header but other contracts do; test

m0ar

Nice 😎

m0ar · 2019-10-28T15:17:37Z

pkg/contract_watcher/header/transformer/transformer.go

 		for _, log := range allLogs {
 			addr := strings.ToLower(log.Address.Hex())
 			sortedLogs[addr] = append(sortedLogs[addr], log)
 		}

 		// Process logs for each contract
 		for conAddr, logs := range sortedLogs {
-			if logs == nil {
+			con := tr.Contracts[conAddr]
+			if len(logs) < 1 {


Any potential consequences with removing the nil check?

The len(logs) substitutes for it and covers both the nil and empty slice case, but there is no situation where the logs will be length 0 without being nil so I can revert it back.

Huh, TIL! Thought that'd lead to a NPE :)

m0ar · 2019-10-28T15:19:04Z

pkg/contract_watcher/shared/getter/interface_getter.go

-	if err != nil {
-		return "", fmt.Errorf("call to getSupportsInterface failed: %v", err)
+	if err != nil || !supports {
+		return ""


Passing on the error makes it possible to differentiate between the call to the client failing and !supports. The empty string makes for a confusing error in the test :)

m0ar · 2019-10-28T15:19:59Z

pkg/contract_watcher/shared/getter/getter_test.go

@@ -47,8 +47,7 @@ var _ = Describe("Interface Getter", func() {
 			transactionConverter := rpc2.NewRpcTransactionConverter(ethClient)
 			blockChain := eth.NewBlockChain(blockChainClient, rpcClient, node, transactionConverter)
 			interfaceGetter := getter.NewInterfaceGetter(blockChain)
-			abi, err := interfaceGetter.GetABI(constants.PublicResolverAddress, blockNumber)
-			Expect(err).NotTo(HaveOccurred())
+			abi := interfaceGetter.GetABI(constants.PublicResolverAddress, blockNumber)


Didn't I recently add this error propagation? If running the tests without the mainnet infura key set, the error is very confusing. This passed along the error from geth instead of making that abi just the empty string.

My mistake, that makes sense- reverting back.

i-norden force-pushed the missed_marked_checked_headers branch 2 times, most recently from 7ed1432 to 20adcc6 Compare October 15, 2019 19:02

i-norden changed the title ~~[WIP] Potential fix for issue #146~~ Fix for issue #146 Oct 15, 2019

i-norden force-pushed the missed_marked_checked_headers branch from 20adcc6 to cd92056 Compare October 24, 2019 18:12

i-norden requested review from rmulhol, elizabethengelman, m0ar, Gslaughl and yaoandrew October 24, 2019 18:28

rmulhol approved these changes Oct 25, 2019

View reviewed changes

i-norden force-pushed the missed_marked_checked_headers branch from 0fa42db to 41ed9a0 Compare October 28, 2019 14:08

i-norden added 3 commits October 28, 2019 09:34

fix for issue #146; mark header checked for contract if it doesnt have

11b5efb

any logs at that header but other contracts do; test

goimports -w; golinting, remove some unused code

6580899

log sql.ErrNoRows which I suspect is what is leading to the flaky test

4fbde83

i-norden force-pushed the missed_marked_checked_headers branch from 41ed9a0 to af5b6cc Compare October 28, 2019 14:41

m0ar approved these changes Oct 28, 2019

View reviewed changes

review fixes

a2d249c

i-norden force-pushed the missed_marked_checked_headers branch from af5b6cc to a2d249c Compare October 28, 2019 16:40

i-norden merged commit b767531 into staging Oct 28, 2019

m0ar deleted the missed_marked_checked_headers branch October 29, 2019 08:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for issue #146 #158

Fix for issue #146 #158

i-norden commented Oct 10, 2019 •

edited

Loading

rmulhol left a comment

rmulhol Oct 25, 2019

i-norden Oct 25, 2019

rmulhol Oct 25, 2019

i-norden Oct 25, 2019

rmulhol Oct 25, 2019

i-norden Oct 28, 2019 •

edited

Loading

m0ar left a comment

m0ar Oct 28, 2019

i-norden Oct 28, 2019 •

edited

Loading

m0ar Oct 29, 2019

m0ar Oct 28, 2019 •

edited

Loading

m0ar Oct 28, 2019

i-norden Oct 28, 2019

		@@ -249,19 +253,32 @@ func (tr *Transformer) Execute() error {
		}

Fix for issue #146 #158

Fix for issue #146 #158

Conversation

i-norden commented Oct 10, 2019 • edited Loading

rmulhol left a 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

i-norden Oct 28, 2019 • edited Loading

Choose a reason for hiding this comment

m0ar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i-norden Oct 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m0ar Oct 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i-norden commented Oct 10, 2019 •

edited

Loading

i-norden Oct 28, 2019 •

edited

Loading

i-norden Oct 28, 2019 •

edited

Loading

m0ar Oct 28, 2019 •

edited

Loading