Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
@jochem-brouwer cool, great that you gave this a start! 😄 👍 🎉 Yeah, it will very much make sense to further abstract here. Let's please give it a cut here, maybe you've got some time during the day and jump on a quick call (let me know when in chat)? It actually get's hyper-tricky here. I guess the main problem is that the concept of inheritance just doesn't work well with this very abstract typed transaction EIP concept where a tx - theoretically - could contain anything. This might work for now - also e.g. regarding the question what is currently contained in But one can already see where this leads to: in EIP-1559 there is e.g. no Not sure if this overloads us for now: but I at least theoretically wonder if e.g. some kind of module concept (gas module, signature module, access list module, eventually serialization module) might serve us better here and will be more future proof. And a base transaction (or however it's called) might contain the information what kind of modules are activated, like so: class BaseTransaction {
modules: {
gas: 'EIP1559',
accessLists: true,
signature: 'ECDSA', // future
serialization: 'RLP',
}Something like this would also allow us to cleaner react in third-party code (VM, Block,...) to features available. The current approach will otherwise further down the road lead to a conditional hell like either always uncleanly just request for some thought-to-be-active properties ( Again, let's just jump on the call sometime. If we do this later the day eventually the others want to join as well. |
|
For reference: Base fee tests in Besu |
|
Nethermind EIP-1559 implementation: NethermindEth/nethermind#2341 |
|
Rebased this one |
|
Yeah this design is getting rather complex already, I can fix this build error but the idea I got in mind might not be a really great design - will take a look at the other implementations first to get some inspiration here. |
|
@jochem-brouwer yeah, this is really tricky to answer and what level we want/need to refactor the whole library for this. In doubt just put this aside for now for a couple of days as some suggestion 😋 , sometimes this helps as well and we can pick this question up on our call and discuss together. If you would have alternatively some time for #1132 that would e.g. also be cool if we get this going a bit again. 😄 |
|
Okay, added some very basic VM tests, but at this point I cannot continue until I have more test data. I will probably create some very simple EIP1559 tests to just see that it produces something (so I can sign a transaction for instance), but I cannot be sure if we are correct. |
jochem-brouwer
force-pushed
the
eip1559
branch
2 times, most recently
from
6af1377 to
52cf035
Compare
|
Rebased upon master. |
|
Ok great, tests pass, now trying to connect to Aleut. |
|
We are only downloading 1 block at a time since for some reason the bootnode either sends invalid data, or we have a bug in devp2p. It tries to decode RLP message bodies where RLP states it should decode ~33k bytes, but the block is actually only 1023 bytes long. (The message body itself is 1024 bytes). |
|
Ok, this commit 8c112f0 fixes the consensus bug at block 55. The problem had nothing to do with EIP1559, it had to do with We should add a test to ensure that clique blocks with txs pay the right miner, which is not tested in VM currently. There also still is a problem with the Nethermind bootnode. If we request more than 1 block (might be ok to request 2 or 3) - lets say the original 50, then we get a RLP decode error on devp2p. With these fixes we can sync up to block 3704: |
|
Another note; we only connect to Nethermind, other 3 bootnodes are up, have to figure out what is the cause of this. http://eip1559-testnet.ops.pegasys.tech:3001/ |
|
"Another node; we only connect to Nethermind" You're too much in @jochem-brouwer. 😂 I've taken the freedom to correct. 😄 |
| common: this.config.chainCommon, | ||
| validateBlocks: true, | ||
| validateConsensus: false, | ||
| validateConsensus: true, |
There was a problem hiding this comment.
This is putting too much load on the PoW sync, I guess what we can do is just to activate for PoA by including a conditional switch along the common instance setting from above.
|
@ryanio hmm, still not sure if #1222 is fixing the problem, I did a rebase here locally and removed 42d83d0 (so mainly the constant bootstrap retriggering, setting Phew, tricky thing. |
|
@holgerd77 this is tricky indeed, I rebased this branch on master (didn't touch the commit you specified) and was able to recreate the issue again. I have a feeling it's still some kind of promise race condition we haven't handled properly. As a test I added a 5s timeout after starting the servers but before calling bootstrap ( ...
await Promise.all(this.config.servers.map((s) => s.start()))
// wait 5s
function timeout(ms: number) {
return new Promise(resolve => setTimeout(resolve, ms));
}
await timeout(5000)
await Promise.all(this.config.servers.map((s) => s.bootstrap()))
...and that worked for me! I got a We have the |
…e-guarding with a new hardforkByHeadBlockNumber option (default: false)
… protocol is available and bound
…adBlockNumber option
|
Guys, I will now merge this in here, tests are passing and I've given all code parts another final look. I have also written extensive tests for the tx library to lift the coverage up again to a decent level and have given all changes in the various libraries at least a rough and often somewhat thorough review. We also do not have an imminently urgent release, at the same time more and more is worked "around" this one huge PR and all the other smaller work is blocked to a greater extend. Everyone can still do reviews on parts where he feels the need, then please integrate your change requests along follow-up PRs. Hope you can go along, but I have once again the feeling that we are at some point where things are not getting better by pushing this ahead. 😋 😀 Thanks @jochem-brouwer for this great work here! 🎉 |
|
Phew, 86 commits (and all very much decent ones, and not "fixed linting", "minor changes", "forgot to add x" 😛 ). |
| gitRevision: 'master', | ||
| exclude: 'test/**/*.ts', | ||
| exclude: [ | ||
| 'src/util.ts', |
There was a problem hiding this comment.
Why do we exclude this?
There was a problem hiding this comment.
The API of this feels too unfinished to me and I would like to keep this internal for now and give this some more thought and have some more discussions around it. I think no one every really reviewed these code parts (including me) and this would expose a huge new API surface.
Some things I would like to discuss (but which doesn't need to be discussed now, just for the mention):
- Is this the best way to expose this functionality or might there be better - eventually more integrated - solutions (e.g. to re-bind the methods to the affected tx classes, just as some first idea)?
- Relationship of this
getAccessListData()(returningAccessListJSONas one of two items) and thegetAccessListJSON()method - Should
getDataFeeEIP2930()be exposed (I have the base feeling that methods which need aCommonas an input often point to some suboptimality in design - we had these cases in the past - can't really point towards a concrete solution thought atm yet)?
I will actually open this as an issue, otherwise it will likely be too tempting to start the discussion here. 😄
3 participants
This PR implements EIP 1559.