Trie stress test #981
Trie stress test #981
Conversation
nytzuga
force-pushed
the
trie-stress-test
branch
4 times, most recently
from
374e3bf to
f520abe
Compare
core/bench_contract_test.go
Outdated
| for { | ||
| gas -= gasTx | ||
| if gas < gasTx { | ||
| break | ||
| } | ||
| tx, _ := types.SignTx(types.NewTransaction(gen.TxNonce(benchRootAddr), contractAddr, big.NewInt(0), gasTx, gasPrice, txPayload), signer, testKey) | ||
| gen.AddTx(tx) | ||
| } | ||
| } |
There was a problem hiding this comment.
I'd prefer to remove the gas calculation in favor of leaving it to the caller to ensure that the parameters they specify don't exceed the gas limit.
In my opinion, it's simpler to do it this way because it removes a bit of logic that is not necessary for the test and it's very reasonable to make that the responsibility of the caller imo.
There was a problem hiding this comment.
Is it better now @aaronbuchwald? The gas calculation is to split the contract creations into multiple blocks, but no other gas calculation is being used.
nytzuga
force-pushed
the
trie-stress-test
branch
from
32851e7 to
154bb9c
Compare
nytzuga
force-pushed
the
trie-stress-test
branch
2 times, most recently
from
533e771 to
920f16c
Compare
There was a problem hiding this comment.
is this file required? if not can we remove it?
There was a problem hiding this comment.
That's the source code of the smart contract. I think it should be included but it is your call. @ceyonur . What should be done?
There was a problem hiding this comment.
This file is not being directly used (compiled files like .abi and .bin used) in tests. This really doesn't fit the purpose of contracts folder which is to provide examples/libraries for other smart contracts on chain. So we should either remove this, or if we want to keep it we should move all of these to a subpackage under core like core/trietest (assuming we don't need any unexported stuff from core package)
There was a problem hiding this comment.
I'm moving to their folder inside core as suggested.
There was a problem hiding this comment.
I meant moving the test as well to the same subpackage.
There was a problem hiding this comment.
@ceyonur functions inside the tests files cannot be exported unfortunately. Unless a bigger refactor is involved this test must be inside core /cc @aaronbuchwald
This smart contract will append N new elements to a vector, this is a stress test to our implementation of the trie structure
Make bench more generic and simpler to bootstrap
Remove gas calculation from benchmarks, gas is still calculated to make sure all contracts are deployed. stressTestTrieDb takes 3 arguments. The number of contracts to deploy, the calls for each block and the numbers of elements to append to the vector on each call
This function will treat execution errors as errors as well, useful for testing. ApplyTransaction has been modified to return the ExecutionResult
Breaking API changes should be avoided
nytzuga
force-pushed
the
trie-stress-test
branch
from
920f16c to
380e198
Compare
Use existing code instead, reading the receipt from the last execution instead of returning the exceutionresult struct
|
I just want to make sure we're on the same page with benchmark target, this looked a bit static for me. |
Co-authored-by: aaronbuchwald <[email protected]> Signed-off-by: Cesar <[email protected]>
Why this should be merged
Write a benchmark with a smart contract that will do a lot of inserts in the TrieDB to test and measure our improvements.
How this works
A smart contract has been created that inserts N number of elements in each transaction. The main goal is to effortlessly to make the EVM do a lot of inserts in the triedb and measure their performance.
How this was tested
How is this documented