1559 rpc #22964
1559 rpc #22964
Conversation
holiman
requested review from
karalabe,
rjl493456442 and
zsfelfoldi
as code owners
internal/ethapi/transaction_args.go
Outdated
| if gasPrice == nil && feeCap == nil && tip == nil { | ||
| log.Warn("At least one of gasPrice, feeCap, tip should be set. Defaulting to 0 gas price.") | ||
| gasPrice = new(big.Int) | ||
| } |
There was a problem hiding this comment.
This seems wrong to me. Shouldn't it be either gasPrice or both feeCap and tip ?
There was a problem hiding this comment.
I believe my reasoning for this was to allow some of the RPC methods that don't care about gas price (I think trace_call is an example) to not need to specify any fee mechanics. So the idea is to default gasPrice to 0 if no fee parameters are defined. I think this was the behaviour before 1559 txs were introduced.
There was a problem hiding this comment.
Ok, I think we need to be explicit here. These if clauses are too convoluted and hard to reason (see error below). IMHO we should handle. (legacytx, 1554tx, and if both is missing do something meaningful).
There was a problem hiding this comment.
I totally agree it's a bit convoluted now. Do you have any particular scheme in mind? My last commit just adds more convolution into the mix
internal/ethapi/api.go
Outdated
| } | ||
| return result | ||
| } | ||
|
|
||
| // newRPCPendingTransaction returns a pending transaction that will serialize to the RPC representation | ||
| func newRPCPendingTransaction(tx *types.Transaction) *RPCTransaction { | ||
| return newRPCTransaction(tx, common.Hash{}, 0, 0) | ||
| return newRPCTransaction(tx, common.Hash{}, 0, 0, nil) |
There was a problem hiding this comment.
We kind of know the base fee for the pending block, don't we? Shouldn't we set that?
There was a problem hiding this comment.
I don't think we do, do we?
There was a problem hiding this comment.
Since the base fee depends on the parent block, we should be able determine the base fee for the pending block by calculating the base fee using the current block?
There was a problem hiding this comment.
Since we can calculate the pending bloxk's basefee, I think we should.
There was a problem hiding this comment.
Ok, pushed a commit, PTAL
holiman
force-pushed
the
1559-rpc
branch
2 times, most recently
from
a9ec536
to
aaeeb54
Compare
|
Rebased, fixed some of the concerns. My questions are still to be resolved, though |
eth/gasprice/gasprice.go
Outdated
| func (t transactionsByGasPrice) Less(i, j int) bool { return t[i].FeeCapCmp(t[j]) < 0 } | ||
| func (t transactionsByTip) Len() int { return len(t) } | ||
| func (t transactionsByTip) Swap(i, j int) { t[i], t[j] = t[j], t[i] } | ||
| func (t transactionsByTip) Less(i, j int) bool { return t[i].TipCmp(t[j]) < 0 } |
There was a problem hiding this comment.
Why should we sort the transaction by the tip in order to suggest a suitable tip?
I think we should sort by effective_tip.
There was a problem hiding this comment.
I think the price oracle part is not correct.
All we need to change is to return a suggested "effective_tip". For the legacy transactions, this effective_tip can be used as theGasPrice directly; for the 1559 transactions, this effective_tip can be used as the tip field.
We don't need this two APIs SuggestPrice and SuggestTip. We should make the result suitable for both legacy transactions and 1559.
eth/api_backend.go
Outdated
| } | ||
|
|
||
| func (b *EthAPIBackend) SuggestFeeCap(ctx context.Context) (*big.Int, error) { | ||
| return new(big.Int).Mul(b.CurrentBlock().BaseFee(), common.Big2), nil |
There was a problem hiding this comment.
If the network is congested, then at the worst case, the transaction can't be included after 6 blocks.
>>> 1.125 ** 6
2.0272865295410156I'm not sure what's the best factor here. Perhaps we can use the gpo for some congestion information. For example if all the previous blocks are fulled with transactions, then this number can be a bit larger.
But for the first version, this hard coded number LGTM. Just throw out the thoughts here.
There was a problem hiding this comment.
It seems like this is a good direction and something that @MicahZoltu brought up here: ethereum/execution-specs#201 (comment)
There was a problem hiding this comment.
Recommend reading the linked discussion for full context, but my preference is that Ethereum clients don't suggest a fee cap to users because users will use it out of habit instead of changing their behavior to what is optimal for them and the network, which is to set the fee cap to their willingness to pay.
| result.FeeCap = (*hexutil.Big)(tx.FeeCap()) | ||
| result.Tip = (*hexutil.Big)(tx.Tip()) | ||
| // if the transaction has been mined, compute the effective gas price | ||
| if baseFee != nil && blockHash != (common.Hash{}) { |
There was a problem hiding this comment.
This might be worth a cross client sync, just so we behave similar to others (e.g. do we or do we not return gasPrice for pending txs)?
internal/ethapi/transaction_args.go
Outdated
| } | ||
| // If baseFee provided, set gasPrice to effectiveGasPrice. | ||
| if baseFee != nil { | ||
| gasPrice = math.BigMin(new(big.Int).Add(tip, baseFee), feeCap) |
There was a problem hiding this comment.
If I have a baseFee and gasPrice != nil && tip == nil && fee == nil, I'd expect this to ether blow up, or be very incorrect :)
There was a problem hiding this comment.
Yeah, it's gonna panic https://play.golang.org/p/Xxfr6yqdwNK
There was a problem hiding this comment.
Bleh, this method is so messy, particularly since it doesn't return an error
There was a problem hiding this comment.
I'm not convinced it's ok though. If gasPrice is specified, it should be treated as a legacy tx and not overridden.
There was a problem hiding this comment.
Here's a counter proposal that should clean things up a bit https://gist.github.com/karalabe/91716ea8ae7e7e9fa81ffcee7d68a8b4
There was a problem hiding this comment.
@karalabe this is to simulate the behaviour in Transaction.AsMessage
go-ethereum/core/types/transaction.go
Lines 608 to 611 in 2cde472
I believe this is only used to propogate the effectiveTip to the gasprice opcode. It may be better to just have this implemented in the state_transition logic rather than AsMessage?
There was a problem hiding this comment.
The idea is that it's ok to try and be smart and guess non-specified fields, but if we got invalid combos, I feel it's a bad idea to "fix it" automagically, we should just reject it.
|
Does this line should have a |
|
@fvictorio yes, I started that little change (setting londonblock in xxAllProtocolChanges) in this PR: #22901 Unfortunately, doing so opens up a huge can of bugs, so it's better to not do it until the base is stabilised. |
…Transaction results
Replaces #22834
I'm a bit unsure about the
gpo, and how we want it to behave.