Support avalanchego usage #2
Conversation
builder.go
Outdated
| // TODO: This shouldn't be done immediately, there should be some | ||
| // retry delay if block building failed. | ||
| if pool.Len() > 0 { | ||
| select { | ||
| case vm.toEngine <- snowcommon.PendingTxs: | ||
| default: | ||
| p := snowcommon.PendingTxs | ||
| vm.logger().Info(fmt.Sprintf("%T(%s) dropped", p, p)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Without this, we will only attempt to build a block if a new tx is issued. We won't attempt to build a block once we are ready to settle a new state (and give us space to issue new txs)
builder.go
Outdated
| if gasUsed == 0 && len(txs) == 0 { | ||
| vm.logger().Info("Blocks must either settle or include transactions") | ||
| return nil, fmt.Errorf("%w: parent %#x at time %d", errBlockWithoutChanges, parent.Hash(), timestamp) | ||
| } |
There was a problem hiding this comment.
Without this check, we would produce an empty block for every BuildBlock attempt where the queue was full. When we did this upon a new tx being issued, the number of empty blocks was kind of hard to see, but once we introduced retries I ended up accepting thousands of blocks almost immediately haha.
There was a problem hiding this comment.
Already aware of this, but please keep adding temp fixes like this just in case I've missed something.
There was a problem hiding this comment.
Blocks must either settle or include transactions
What's our decision here? I'm ok with allowing settlement without new transactions as long as you are. If anyone is depending on the "safe" block then they'll appreciate it. That said, if there are no transactions in the mempool then the gas price will drop and there will be arb opportunities that will result in some.
There was a problem hiding this comment.
I think we ban empty blocks to start
vm.go
Outdated
| HTTPHandlerKey = "/sae/http" | ||
| WSHandlerKey = "/sae/ws" |
There was a problem hiding this comment.
The previous paths were invalid
builder.go
Outdated
| if gasUsed == 0 && len(txs) == 0 { | ||
| vm.logger().Info("Blocks must either settle or include transactions") | ||
| return nil, fmt.Errorf("%w: parent %#x at time %d", errBlockWithoutChanges, parent.Hash(), timestamp) | ||
| } |
There was a problem hiding this comment.
Already aware of this, but please keep adding temp fixes like this just in case I've missed something.
builder.go
Outdated
| @@ -173,6 +196,9 @@ TxLoop: | |||
|
|
|||
| validity, err := checker.addTxToQueue(candidate.txAndSender) | |||
| if err != nil { | |||
| // TODO: It is not acceptable to return an error here, as all | |||
There was a problem hiding this comment.
ACK
FYI, I've completely replaced validity.go with the worstcase package but builder.go still has this problem.
There was a problem hiding this comment.
Fixed with the defer that I merged a few lines above. Still returns an error but replaces the tx.
There was a problem hiding this comment.
Shouldn't we just log this and drop the tx?
…vm into StephenButtolph/strevm-poc
ARR4N
force-pushed
the
arr4n/strevm-poc
branch
4 times, most recently
from
df735f7 to
84fb6a8
Compare
ARR4N
force-pushed
the
arr4n/strevm-poc
branch
2 times, most recently
from
59dab43 to
b895b4d
Compare
The `go.yml` and `lint.yml` CI workflows come from `libevm` as reasonable first approximations of what we want for this repo. The failures on the Go workflow are all to be expected because there aren't any Go files to test, generate, nor lint. What's important is that all workflows are running.
`goheader` only checks new issues in `libevm` because `MOD-YEAR-RANGE` breaks otherwise (we can reintroduce this next year if we haven't moved the code into `avalanchego` yet). The other deleted config is what `libevm` uses to avoid linting `geth` code. Go workflows are still expected to fail, as described in #3, and all that matters is that they run (and `golangci-lint` _attempts_ to lint but finds no files).
As it says on the tin.
Allows block implementations to be read-only and not have to be aware of
the VM's that created them, which is typically necessary when calling
`Block.Verify()`, `Accept()`, or `Reject()`. These methods instead exist
on the VM as `{Verify,Accept,Reject}Block()`.
There are no explicit tests for this package as it's exercised heavily
via the actual SAE tests that will follow.
This package forms the basis of the gas clock, which will extend a `proxytime.Time[gas.Gas]` in a future PR. The `Time.SetRateInvariants()` functionality will be used for changing gas target without affecting gas price, by scaling rate, target, and excess proportionally. All other functionality is necessary for the execution thread. Although the `CmpOpt()` configuration option might feel like overkill, it's necessary for the more extensive, system-wide tests that come later. --------- Signed-off-by: Arran Schlosberg <[email protected]> Co-authored-by: Stephen Buttolph <[email protected]>
No description provided.