server/eth: Match fee rate = MaxFeeRate

[martonp]

• server/asset: Add Info and ValidateFeeRate functions
• server/dex: Add SwapFeeRate function to FeeFetcher. This will return the max fee rate for assets that support dynamic transaction fees, but the regular tx fee otherwise
• server/market: Use SwapFeeRate instead of the regular FeeRate
• server/swap: Use ValidateFeeRate function instead of just comparing the fee rates. This is to allow ETH to check the prority fee in addition to the MaxFeeCap

Closes #1372.

[JoeGruffins]

Working well.

[chappjc]

LGTM. Keeping open to see if @buck54321 is good with SupportsDynamicTxFee + ValidateFeeRate

[buck54321]

Concept ACK. Untested and one suggestion, but feel free to merge when you're comfy, @chappjc.

[buck54321]

We should probably just enforce dynamicTx = true for all of our transactions.

[buck54321]

Wouldn't need to implement this here and in dcr if you made SupportsDynamicTxFee part of an optional interface like we're doing with the client.

[buck54321]

Actually, I'm now thinking this might not work like I expected, since it's just a boolean return value. So we'd verify that the interface is a DynamicFeeSupporter or whatever, and then still check the boolean? What we really need is something akin to WalletInfo, but on the server side. I'm okay with this as is too

[chappjc]

Yeah, it works as is. @martonp can try some stuff out I guess, but I can see the optional method being SwapFeeRate, which would make feeFetcher unaware of the whole dynamic<->max_fee relationship. Maybe we like that dynamic/max concept front and center?

Specifically, changing

// SwapFeeRate returns the tx fee that needs to be used to initiate a swap.
// This fee will be the max fee rate if the asset supports dynamic tx fees,
// and otherwise it will be the current market fee rate.
func (f *feeFetcher) SwapFeeRate(ctx context.Context) uint64 {
	if f.Backend.SupportsDynamicTxFee() {
		return f.MaxFeeRate()
	}
	return f.FeeRate(ctx)
}

into

// SwapFeeRate returns the tx fee that needs to be used to initiate a swap.
// This will use the Backend's SwapFeeRate method if it exists, otherwise
// it will request the fee rate based on current network conditions.
func (f *feeFetcher) SwapFeeRate(ctx context.Context) uint64 {
	if fr, ok := f.Backend.(asset.SwapFeeRater); ok {
		return fr.SwapFeeRate()
	}
	return f.FeeRate(ctx)
}

The question is do we hide the whole dynamic-must-use-max concept in the Backend's methods or do we apply that notion at the higher levels like done currently in this PR?

[martonp]

This wouldn't work because the backend doesn't have access to the MaxFeeRate. It's stored in the dex.Asset.

[chappjc]

Ahh, that's right. Max fee rate is a market/swapper concept, so must be done higher up. ✔️

[chappjc]

Concept ACK. Untested and one suggestion, but feel free to merge when you're comfy, @chappjc.

I like both of your suggestions, enforce dynamic tx and consider an interface assertion for server's asset Backends

[martonp]

We don't really need to check for dynamic vs legacy, in the geth Transaction object, for a legacy transaction GasFeeCap and GasTipCap will both just return the gas price. GasPrice returns the fee cap in a dynamic transaction. Anyways, the code is more simple like this. I also added a test to make sure the coiner was populating the tip cap correctly.

[chappjc]

@martonp I noticed that too:

https://github.com/ethereum/go-ethereum/blob/aaca58a7a1d9acbd24bbc74c49933efa2f1af183/core/types/legacy_tx.go#L99-L101

func (tx *LegacyTx) gasPrice() *big.Int     { return tx.GasPrice }
func (tx *LegacyTx) gasTipCap() *big.Int    { return tx.GasPrice }
func (tx *LegacyTx) gasFeeCap() *big.Int    { return tx.GasPrice }

vs.

func (tx *DynamicFeeTx) gasFeeCap() *big.Int    { return tx.GasFeeCap }
func (tx *DynamicFeeTx) gasTipCap() *big.Int    { return tx.GasTipCap }
func (tx *DynamicFeeTx) gasPrice() *big.Int     { return tx.GasFeeCap }

accessed via

// GasPrice returns the gas price of the transaction.
func (tx *Transaction) GasPrice() *big.Int { return new(big.Int).Set(tx.inner.gasPrice()) }

// GasTipCap returns the gasTipCap per gas of the transaction.
func (tx *Transaction) GasTipCap() *big.Int { return new(big.Int).Set(tx.inner.gasTipCap()) }

// GasFeeCap returns the fee cap per gas of the transaction.
func (tx *Transaction) GasFeeCap() *big.Int { return new(big.Int).Set(tx.inner.gasFeeCap()) }

But your point is that if it's a legacy txn with a gasPrice set to our (very high) gasFeeCap, so be it, even though that's incredibly wasteful of the user?

So you are proposing setting baseCoin.gasFeeCap to either gasPrice if it is a legacy txn or actually gasFeeCap for dynamic. Seems reasonable, and if we do decide to block legacy txns, we can always go back to (*Backend).baseCoin to block it.

Finally, given the above code snippets, was our way of detecting legacy/dynamic tx type incorrect before? Should we have been using (tx *Transaction) Type() uint8?

[martonp]

But your point is that if it's a legacy txn with a gasPrice set to our (very high) gasFeeCap, so be it, even though that's incredibly wasteful of the user?

Yes, I think the client should definitely use a dynamic transaction, but I don't see a reason to block legacy ones.

Finally, given the above code snippets, was our way of detecting legacy/dynamic tx type incorrect before? Should we have been using (tx *Transaction) Type() uint8?

Yeah, what we had was pointless lol.

[chappjc]

With a legacy transaction, which returns the same gas price for both GasFeeCap and GasTipCap, I think that the swapCoin.gasTipCap check in (*Backend).ValidateFeeRate is wrong or at least misleading.
Say we are debugging and we inspect a swapCoin, we'd see a gasTipCap == gasFeeCap (a huge gasTipCap).
I guess we can solve this with documentation spelling out the logic of why a legacy txn can be treated as a dynamic tx with a massive tip, but I do think we should spell that out in both methods (baseCoin construction and ValidateFeeRate)

[martonp]

I added some docs.