-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(node): add anvil node #1037
Conversation
should we rebase this to master so that it doesn't have the revm commits? |
yeah will fixup all revm related commits |
I see a tracing service mentioned in the commits. Fwiw Besu has support for open telemetry if your looking for a reference that is useful. |
b84b857
to
d369b1e
Compare
Random mind dump from my accumulation of building this in cevm back in the day (see here):
|
thanks for this. |
Not explicitly required, but we should probably strive for compatibility with |
Perhaps its a good idea to work with the hardhat team to standardize some RPCs for these kinds of actions so that tests can be written in a portable way. Right now hardhat has a |
re: some comments on one of my ethers changes I've been diving into this PR, I wanted to see how much we could share with ethers, and comment on what's missing w.r.t transaction types. Based on some messing around with types (link to a rough PoC on top of this PR), I think the node presents a need for a signed request type, as opposed to the existing Quick primer on the differences between these types, and what we needThe ethers
The ethers
In the node, we need to work with signed transaction requests in a ton of places, and use ethers_core::types::{
transaction::eip2718::TypedTransaction,
Signature,
};
/// Represents a signed transaction request
pub struct SignedTransaction {
pub tx: EthersTypedTransaction,
pub signature: Signature,
} We could introduce this type into ethers and move the decoding changes in ethers-rs#1096 onto that type. The PoC removes most of the transaction types introduced in the node. What do you all think about adding something like this in ethers so we can simplify things in the node? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approved - modulo my comments on integration tests against uniswap v3-core not passing + how should we be thinking about the waffle patch required in v3-periphery
The only tests failing for uni v3 are currently not sure why a) happens, could just be differences in how revm/hh measure. b) happens because waffle has hardhat decoding baked in https://github.com/TrueFiEng/Waffle/blob/master/waffle-chai/src/matchers/revertedWith.ts#L80 rn the hardhat-waffle plugin is only compatible with hardhat due to this I made a drop-in replacement plugin here: https://github.com/mattsse/hardhat-anvil this currently lacks proper anvil config that could be massively simplified if the network name would not be enforced in hardhat-waffle |
Let's get it 🔥🔥🔥🔥 BIG milestone |
@@ -249,7 +256,10 @@ where | |||
trace!(target: "backendhandler", "last sender dropped, ready to drop (&flush cache)"); | |||
return Poll::Ready(()) | |||
} | |||
_ => break, | |||
Poll::Pending => { | |||
cx.waker().wake_by_ref(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm confused, what does it do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why, but in node fork mode, the task did not wake up after reaching this state, even if new messages were sent to the channel...
this explicitly tells the scheduler to poll it again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there an easy way to reproduce it? there is definitely something fishy with spawning and hyper, but I processed ~8 months worth of blocks without this change.
just tested it and this single line cuts the throughput in half.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because of that (probably) hyper problem i'm using foundry-evm with Ws
provider & most of the processing lives in futures::executor::ThreadPool::spawn_with_handle
, so maybe perf degradation is not visible in foundry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeh this a rather poor solution and turns this basically into a busy loop.
performance for foundry itself is not really an issue here, so because I wasn't able to track this down made this change.
is there an easy way to reproduce it?
I believe it was this test or any other fork test
https://github.com/foundry-rs/foundry/blob/master/anvil/tests/it/fork.rs#L34
just tested again and it hangs when the SharedBackend sends the request if I remove the wake, not sure why though
also tested with increased buffer size and UnboundedSender/Receiver, same result
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@univerz realize I'm hijacking the thread but if you wanna join the foundry telegram group to have more synchronous conversations, it'd be great! https://t.me/foundry_rs
maybe forge create could deploy contracts to anvil by default? |
Motivation
Add support for
forge node
Solution