Reconciliation failed for genesis block account

[tuxcanfly]

Describe the bug
I have a blockchain which contains a transaction in the genesis block. This transaction contains a single output going to an address. I have found that the reconciliation step fails, expecting
computed balance to be twice the node balance (which is simply just the only output from the genesis transaction). The failure log shows, for example:

Reconciliation failed for ss1q7q3h4chglps004u3yn79z0cp9ed24rfr9dn5nv at 0 computed: 4004420000HNS node: 2002210000HNS

To Reproduce

• Create a genesis block containing a transaction generating an output to an address.

• Run rosetta-cli check

Expected behavior

• Check fails with the reconciliation expecting a balance equal to double the node balance.

Additional context

rosetta-cli version v0.2.3, go version v0.2.3 on ubuntu linux, bionic

[tuxcanfly]

https://github.com/coinbase/rosetta-cli/blob/master/internal/storage/block_storage.go#L501

	} else {
		amount, err := b.helper.AccountBalance(ctx, change.Account, change.Currency, parentBlock)
		if err != nil {
			return fmt.Errorf("%w: unable to get previous account balance", err)
		}

		existingValue = amount.Value
	}

If change.Block and parentBlock are both genesis blocks, then this code assigning existingValue to the amount.Value might explain the reason for the computed balance being twice the node balance.

[patrick-ogrady]

You are spot on @tuxcanfly. Looks like this regression was introduced in the large syncer/reconciler refactor (#15). While we work on a fix, you can still check all other blocks by running rosetta-cli check --start 1 (assuming the genesis index of the blockchain you are testing is 0).

[tuxcanfly]

Awesome. I was working on fix that involves checking change.Block against parentBlock and initializing existingValue = "0" but perhaps it would be best to pass genesisBlock and compare against that.

[patrick-ogrady]

I was thinking through some potential fixes here as well. I think there are 2 reasonable approaches here:

• (your idea) Set the value to 0 if types.Hash(parentBlock) == types.Hash(change.Block). One downside of this approach is that any accounts with balances set in the genesis file that have a transaction in the genesis block will always error (would not happen with option 2). However, there is an easy workaround here to bootstrap balances using the --bootstrap-balances flag. Still thinking about using the genesisBlock here (could see the benefits of being specific, however, this should never happen in another case).
• Skip balance updates (return nil from the function you copied above) when types.Hash(parentBlock) == types.Hash(change.Block). We will then set the balance later when we can do so correctly. The major issue here is that balances of skipped accounts will be incorrect in the db until they are seen again 😬.

I believe your idea is the "most correct" but it could impose a minor usability hurdle in some edge cases for blockchains with genesis allocations and genesis transactions (I think that tradeoff is fine).

Let me know if you are going to put up a PR for this @tuxcanfly. Otherwise, I'll take a stab at this!

[patrick-ogrady]

Fortunately, it will be pretty straightforward to write a test case here (example test linked below): https://github.com/coinbase/rosetta-cli/blob/1f0d9c523088a77d9f77e03b0d66a31f2d75a563/internal/storage/block_storage_test.go#L371

[patrick-ogrady]

Following up here @tuxcanfly ^^^

[tuxcanfly]

Working on a fix using option 1 and suggestions.