mirror of https://github.com/hyperledger/besu
[NC-1685][NC-1644] BlockHashOperation fixes (#38)
* [NC-1685] Fix BlockHashOperation so it handles blocks being added on forks and gets the hash of the block at the requested number on that fork rather than on the current canonical chain. * [NC-1644] Cache block hashes across all transactions in a block. Signed-off-by: Adrian Sutton <adrian.sutton@consensys.net>pull/2/head
Adrian Sutton
6 years ago
committed by
GitHub
parent
feee715f6f
commit
84d89fe56e
@ -0,0 +1,44 @@ |
||||
package net.consensys.pantheon.ethereum.vm; |
||||
|
||||
import net.consensys.pantheon.ethereum.chain.Blockchain; |
||||
import net.consensys.pantheon.ethereum.core.Hash; |
||||
import net.consensys.pantheon.ethereum.core.ProcessableBlockHeader; |
||||
import net.consensys.pantheon.ethereum.vm.operations.BlockHashOperation; |
||||
|
||||
import java.util.HashMap; |
||||
import java.util.Map; |
||||
|
||||
/** |
||||
* Calculates and caches block hashes by number following the chain for a specific branch. This is |
||||
* used by {@link BlockHashOperation} and ensures that the correct block hash is returned even when |
||||
* the block being imported is on a fork. |
||||
* |
||||
* <p>A new BlockHashCache must be created for each block being processed but should be reused for |
||||
* all transactions within that block. |
||||
*/ |
||||
public class BlockHashLookup { |
||||
|
||||
private ProcessableBlockHeader searchStartHeader; |
||||
private final Blockchain blockchain; |
||||
private final Map<Long, Hash> hashByNumber = new HashMap<>(); |
||||
|
||||
public BlockHashLookup(final ProcessableBlockHeader currentBlock, final Blockchain blockchain) { |
||||
this.searchStartHeader = currentBlock; |
||||
this.blockchain = blockchain; |
||||
hashByNumber.put(currentBlock.getNumber() - 1, currentBlock.getParentHash()); |
||||
} |
||||
|
||||
public Hash getBlockHash(final long blockNumber) { |
||||
final Hash cachedHash = hashByNumber.get(blockNumber); |
||||
if (cachedHash != null) { |
||||
return cachedHash; |
||||
} |
||||
while (searchStartHeader != null && searchStartHeader.getNumber() - 1 > blockNumber) { |
||||
searchStartHeader = blockchain.getBlockHeader(searchStartHeader.getParentHash()).orElse(null); |
||||
if (searchStartHeader != null) { |
||||
hashByNumber.put(searchStartHeader.getNumber() - 1, searchStartHeader.getParentHash()); |
||||
} |
||||
} |
||||
return hashByNumber.getOrDefault(blockNumber, Hash.ZERO); |
||||
} |
||||
} |
||||
@ -0,0 +1,107 @@ |
||||
package net.consensys.pantheon.ethereum.vm; |
||||
|
||||
import static org.assertj.core.api.Assertions.assertThat; |
||||
import static org.mockito.ArgumentMatchers.anyLong; |
||||
import static org.mockito.Mockito.mock; |
||||
import static org.mockito.Mockito.never; |
||||
import static org.mockito.Mockito.verify; |
||||
import static org.mockito.Mockito.verifyNoMoreInteractions; |
||||
import static org.mockito.Mockito.verifyZeroInteractions; |
||||
import static org.mockito.Mockito.when; |
||||
|
||||
import net.consensys.pantheon.ethereum.chain.Blockchain; |
||||
import net.consensys.pantheon.ethereum.core.BlockHeader; |
||||
import net.consensys.pantheon.ethereum.core.BlockHeaderTestFixture; |
||||
import net.consensys.pantheon.ethereum.core.Hash; |
||||
|
||||
import java.util.Optional; |
||||
|
||||
import org.junit.After; |
||||
import org.junit.Before; |
||||
import org.junit.Test; |
||||
|
||||
public class BlockHashLookupTest { |
||||
|
||||
private static final int CURRENT_BLOCK_NUMBER = 256; |
||||
private final Blockchain blockchain = mock(Blockchain.class); |
||||
private final BlockHeader[] headers = new BlockHeader[CURRENT_BLOCK_NUMBER]; |
||||
private BlockHashLookup lookup; |
||||
|
||||
@Before |
||||
public void setUp() { |
||||
BlockHeader parentHeader = null; |
||||
for (int i = 0; i < headers.length; i++) { |
||||
final BlockHeader header = createHeader(i, parentHeader); |
||||
when(blockchain.getBlockHeader(header.getHash())).thenReturn(Optional.of(header)); |
||||
headers[i] = header; |
||||
parentHeader = headers[i]; |
||||
} |
||||
lookup = |
||||
new BlockHashLookup( |
||||
createHeader(CURRENT_BLOCK_NUMBER, headers[headers.length - 1]), blockchain); |
||||
} |
||||
|
||||
@After |
||||
public void verifyBlocksNeverLookedUpByNumber() { |
||||
// Looking up the block by number is incorrect because it always uses the canonical chain even
|
||||
// if the block being imported is on a fork.
|
||||
verify(blockchain, never()).getBlockHeader(anyLong()); |
||||
} |
||||
|
||||
@Test |
||||
public void shouldGetHashOfImmediateParent() { |
||||
assertHashForBlockNumber(CURRENT_BLOCK_NUMBER - 1); |
||||
} |
||||
|
||||
@Test |
||||
public void shouldGetHashOfGenesisBlock() { |
||||
assertHashForBlockNumber(0); |
||||
} |
||||
|
||||
@Test |
||||
public void shouldGetHashForRecentBlockAfterOlderBlock() { |
||||
assertHashForBlockNumber(10); |
||||
assertHashForBlockNumber(CURRENT_BLOCK_NUMBER - 1); |
||||
} |
||||
|
||||
@Test |
||||
public void shouldReturnEmptyHashWhenRequestedBlockNotOnChain() { |
||||
assertThat(lookup.getBlockHash(CURRENT_BLOCK_NUMBER + 20)).isEqualTo(Hash.ZERO); |
||||
} |
||||
|
||||
@Test |
||||
public void shouldReturnEmptyHashWhenParentBlockNotOnChain() { |
||||
final BlockHashLookup lookupWithUnavailableParent = |
||||
new BlockHashLookup( |
||||
new BlockHeaderTestFixture().number(CURRENT_BLOCK_NUMBER + 20).buildHeader(), |
||||
blockchain); |
||||
assertThat(lookupWithUnavailableParent.getBlockHash(CURRENT_BLOCK_NUMBER)).isEqualTo(Hash.ZERO); |
||||
} |
||||
|
||||
@Test |
||||
public void shouldGetParentHashFromCurrentBlock() { |
||||
assertHashForBlockNumber(CURRENT_BLOCK_NUMBER - 1); |
||||
verifyZeroInteractions(blockchain); |
||||
} |
||||
|
||||
@Test |
||||
public void shouldCacheBlockHashesWhileIteratingBackToPreviousHeader() { |
||||
assertHashForBlockNumber(CURRENT_BLOCK_NUMBER - 4); |
||||
assertHashForBlockNumber(CURRENT_BLOCK_NUMBER - 1); |
||||
verify(blockchain).getBlockHeader(headers[CURRENT_BLOCK_NUMBER - 1].getHash()); |
||||
verify(blockchain).getBlockHeader(headers[CURRENT_BLOCK_NUMBER - 2].getHash()); |
||||
verify(blockchain).getBlockHeader(headers[CURRENT_BLOCK_NUMBER - 3].getHash()); |
||||
verifyNoMoreInteractions(blockchain); |
||||
} |
||||
|
||||
private void assertHashForBlockNumber(final int blockNumber) { |
||||
assertThat(lookup.getBlockHash(blockNumber)).isEqualTo(headers[blockNumber].getHash()); |
||||
} |
||||
|
||||
private BlockHeader createHeader(final int blockNumber, final BlockHeader parentHeader) { |
||||
return new BlockHeaderTestFixture() |
||||
.number(blockNumber) |
||||
.parentHash(parentHeader != null ? parentHeader.getHash() : Hash.EMPTY) |
||||
.buildHeader(); |
||||
} |
||||
} |
||||
Loading…
Reference in new issue