RSKIP-535 - Addition of Base event in the BlockHeader #3358
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
fmacleal
force-pushed
the
fmacleal/superevent-blockheader-extension
branch
2 times, most recently
from
8093f04 to
0a5090f
Compare
fmacleal
changed the title
fmacleal
force-pushed
the
fmacleal/superevent-blockheader-extension
branch
from
64dc5cb to
0b8c673
Compare
fmacleal
force-pushed
the
fmacleal/superevent-blockheader-extension
branch
6 times, most recently
from
15b20f6 to
e612a04
Compare
rskj-core/src/main/java/org/ethereum/config/blockchain/upgrades/ActivationConfig.java
Outdated
Show resolved
Hide resolved
rskj-core/src/main/java/org/ethereum/core/exception/FieldMaxSizeBlockHeaderException.java
Outdated
Show resolved
Hide resolved
rskj-core/src/main/java/org/ethereum/core/exception/SealedBlockException.java
Outdated
Show resolved
Hide resolved
rskj-core/src/main/java/org/ethereum/core/exception/SealedBlockException.java
Outdated
Show resolved
Hide resolved
rskj-core/src/main/java/org/ethereum/core/exception/SealedBlockHeaderException.java
Outdated
Show resolved
Hide resolved
rskj-core/src/main/java/org/ethereum/core/exception/TransactionException.java
Outdated
Show resolved
Hide resolved
rskj-core/src/main/java/org/ethereum/core/BlockHeaderBuilder.java
Outdated
Show resolved
Hide resolved
fmacleal
force-pushed
the
fmacleal/superevent-blockheader-extension
branch
from
32ea855 to
935b3ab
Compare
|
fmacleal
force-pushed
the
fmacleal/superevent-blockheader-extension
branch
from
fbcb422 to
3d5fa64
Compare
|
|
||
| private void smokeTest(String contentType, String host, InetAddress rpcAddress, List<String> rpcHost, List<ModuleDescription> filteredModules, Function<Config, Config> decorator, String mockResult, String method) throws Exception { | ||
| private void smokeTest(String contentType, String host, InetAddress rpcAddress, List<String> rpcHost, | ||
| List<ModuleDescription> filteredModules, Function<Config, Config> decorator, String mockResult, |
Check notice
Code scanning / CodeQL
Useless parameter Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 days ago
The best way to fix this is to remove the unused decorator parameter from all the overloaded smokeTest methods where it is present, and adjust all calls to these methods as needed. This means:
- Remove
Function<Config, Config> decoratorfrom:- Line 579:
private void smokeTest(...) - Line 585:
private void smokeTest(...) - Line 600:
private void smokeTest(...)
- Line 579:
- Remove the passing of
decoratorargument in all internal calls within these methods. - If these methods are called externally anywhere else (within the shown code), update call sites accordingly.
In this case, only edits to the shown code within rskj-core/src/test/java/co/rsk/rpc/netty/Web3HttpServerTest.java are needed.
| @@ -577,14 +577,14 @@ | ||
| } | ||
|
|
||
| private void smokeTest(String contentType, String host, List<ModuleDescription> filteredModules, | ||
| Function<Config, Config> decorator, String mockResult, String method) throws Exception { | ||
| smokeTest(contentType, host, InetAddress.getLoopbackAddress(), new ArrayList<>(), filteredModules, decorator, | ||
| String mockResult, String method) throws Exception { | ||
| smokeTest(contentType, host, InetAddress.getLoopbackAddress(), new ArrayList<>(), filteredModules, | ||
| mockResult, method); | ||
| } | ||
|
|
||
| private void smokeTest(String contentType, String host, List<ModuleDescription> filteredModules, | ||
| Function<Config, Config> decorator, String mockResult) throws Exception { | ||
| smokeTest(contentType, host, InetAddress.getLoopbackAddress(), new ArrayList<>(), filteredModules, decorator, | ||
| String mockResult) throws Exception { | ||
| smokeTest(contentType, host, InetAddress.getLoopbackAddress(), new ArrayList<>(), filteredModules, | ||
| mockResult, "web3_sha3"); | ||
| } | ||
|
|
||
| @@ -598,7 +594,7 @@ | ||
| } | ||
|
|
||
| private void smokeTest(String contentType, String host, InetAddress rpcAddress, List<String> rpcHost, | ||
| List<ModuleDescription> filteredModules, Function<Config, Config> decorator, String mockResult, | ||
| List<ModuleDescription> filteredModules, String mockResult, | ||
| String method) throws Exception { | ||
| Web3 web3Mock = Mockito.mock(Web3.class); | ||
| Mockito.when(web3Mock.web3_sha3(anyString())).thenReturn(mockResult); |
| if (block != null && header != null && activationConfig.isActive(ConsensusRule.RSKIP535, block.getNumber())) { | ||
| try { | ||
| Repository repo = repositoryLocator.startTrackingAt(header); | ||
| if (repo != null) { |
There was a problem hiding this comment.
Looks like it throws trieNotFoundException if not found https://github.com/rsksmart/rskj/blob/master/rskj-core/src/main/java/co/rsk/db/RepositoryLocator.java#L85
There was a problem hiding this comment.
I think I added this because it was hard to mock during tests. Also it's a protection, since it's out of core control. Do you think we should handle the exception?
| profiler.stop(metric); | ||
| } | ||
|
|
||
| private void setBaseEventIfRskip535IsActive(Block block, BlockHeader header) { |
There was a problem hiding this comment.
should we move this to a helper class? as for header.setTransactionsRoot(BlockHashesHelper.getTxTrieRoot(block.getTransactionsList(), isRskip126Enabled));
| rskip144 = 1 | ||
| rskip351 = 1 |
There was a problem hiding this comment.
These 2 are already part of reed810 so this can be removed
| rskipUMM = 1 | ||
| rskip144 = 1 | ||
| rskip351 = 1 | ||
| rskip535 = 1 |
There was a problem hiding this comment.
If this is part of Vetiver then no need to add it here
| }, | ||
| consensusRules = { | ||
| rskip97 = -1 # disable orchid difficulty drop | ||
| rskipUMM = 1 |
There was a problem hiding this comment.
And this one is part of papyrus
| if (!activationConfig.isActive(ConsensusRule.RSKIP351, blockNumber) || !compressed) { | ||
| if (rlpHeader.size() > r && activationConfig.isActive(ConsensusRule.RSKIP144, blockNumber)) { | ||
| txExecutionSublistsEdges = ByteUtil.rlpToShorts(rlpHeader.get(r++).getRLPRawData()); | ||
| } | ||
|
|
||
| if ((!activationConfig.isActive(ConsensusRule.RSKIP351, blockNumber) || !compressed) && | ||
| (rlpHeader.size() > r && activationConfig.isActive(ConsensusRule.RSKIP144, blockNumber))) { | ||
| txExecutionSublistsEdges = ByteUtil.rlpToShorts(rlpHeader.get(r++).getRLPRawData()); | ||
| if (rlpHeader.size() > r && activationConfig.isActive(ConsensusRule.RSKIP535, blockNumber)) { | ||
| baseEvent = rlpHeader.get(r++).getRLPRawData(); | ||
| } |
There was a problem hiding this comment.
i think this should be reviewed, rskip351 and rskip535 will probably be active at the same time
if thats the case, then the baseEvent wont ever be updated, since the if condition (!activationConfig.isActive(ConsensusRule.RSKIP351, blockNumber)) wont be met
There was a problem hiding this comment.
I would have to take a deeper look, and it's probably easier to add a test. Maybe I added a test where both were active. By looking to this if I don't see why it wouldn't set the baseEvent. RSKIP-351 it's mandatory for PTE and for baseEvent. Because it's the one that add the possibility to have header extension. If RSKIP-351 it's active, the next field in RLP can be the txSublist from PTE or the baseEvent. If PTE it's enabled, we will set it and increment the index from RLP, then we check baseEvent. Why it wouldn't set the baseEvent if PTE it's enabled in this logic here?
| ConsensusRule.RSKIP85, | ||
| ConsensusRule.RSKIP87, | ||
| ConsensusRule.RSKIP88, | ||
| ConsensusRule.RSKIP89, | ||
| ConsensusRule.RSKIP90, | ||
| ConsensusRule.RSKIP91, | ||
| ConsensusRule.RSKIP92, | ||
| ConsensusRule.RSKIP97, | ||
| ConsensusRule.RSKIP98 |
There was a problem hiding this comment.
Can we undo all these changes? I think we have consensus to use one tab (4 spaces) for indentation
| ConsensusRule.RSKIP351, | ||
| ConsensusRule.RSKIP502, | ||
| ConsensusRule.RSKIP529, | ||
| ConsensusRule.RSKIP535, |
There was a problem hiding this comment.
This one should go with Vetiver not reed810
| "hardforkActivationHeights: {", | ||
| " genesis: 0", | ||
| " bahamas: 0", | ||
| " afterBridgeSync: 0,", | ||
| " orchid: 0,", | ||
| " orchid060: 0,", | ||
| " wasabi100: 0", | ||
| " papyrus200: 0", | ||
| " twoToThree: 0", | ||
| " iris300: 0", | ||
| " hop400: 0", | ||
| " hop401: 0", |
There was a problem hiding this comment.
All these tabs don't help with reviewing the actual changes
| ); | ||
| } | ||
| } | ||
| default -> new BlockHeaderV0( |
There was a problem hiding this comment.
| default -> new BlockHeaderV0( | |
| case 0x0 -> new BlockHeaderV0( |
just a suggestion
|
|
||
| @Override | ||
| public void setBaseEvent(byte[] baseEvent) { | ||
| if (baseEvent != null) { |
There was a problem hiding this comment.
should we also check
if (this.sealed) {
throw new SealedBlockHeaderException("trying to alter baseEvent");
}
here? As in BlockHeaderV0
| makeExtension(compressed, extensionData, txExecutionSublistsEdges, baseEvent), compressed); | ||
| } | ||
|
|
||
| private static BlockHeaderExtensionV1 makeExtension(boolean compressed, |
There was a problem hiding this comment.
| private static BlockHeaderExtensionV1 makeExtension(boolean compressed, | |
| private static BlockHeaderExtensionV2 makeExtension(boolean compressed, |
?
| } | ||
|
|
||
| @Test | ||
| void testSetBaseEventWithVeryLargeValueThrowsException() { |
There was a problem hiding this comment.
we should probaby test the first wrong value too (i.e. 129 bytes), wdyt?
There was a problem hiding this comment.
we should probably add tests for decodeHeader for when base event is active, right?
that way we can test the internal flow when decoding from the block factory
| } | ||
|
|
||
| @Test | ||
| void createHeaderWithVersion2AfterRskip351() { |
There was a problem hiding this comment.
| void createHeaderWithVersion2AfterRskip351() { | |
| void createHeaderWithVersion2AfterRskip535() { |
?
| } | ||
|
|
||
| @Test | ||
| void testBridgeEventWithMaxSize() { |
There was a problem hiding this comment.
do we want this? when setting baseEvent calling blockHeader.setBaseEvent we dont allow large values
There was a problem hiding this comment.
check test names, a bunch of them say bridgeEvent instead of baseEvent
nagarev
dismissed stale reviews from marcos-iov, italo-sampaio, and themself
via
40f1e00
|
This PR adds the BlockHeader extension to save the BridgeEvent. This is essential for the union bridge verification from the pegin events.
This PR still a work in progress.
Description
Motivation and Context
How Has This Been Tested?
Types of changes
Checklist: