[critical] bids get stuck behind deal txs
Closed this issue · 7 comments
The keeper has support for overriding bid txs in handle_bid using replace= in transact_async, which is good. However, the keeper is not able to override deal txs at all: if a deal tx is made then the keeper will be completely stuck until it is mined:
https://github.com/makerdao/auction-keeper/blob/master/auction_keeper/main.py#L430
Two examples of deal txs in the wild making subsequent bids stuck:
https://etherscan.io/tx/0x675905cd4a561973a62d6f50f8719a9ec3c578c0071ebb53033bd274d05cbfbd
https://etherscan.io/tx/0xc76d400119206aa7a41637da29a21f9c5b271d36dcf00e13872e1943ec1694d8
Solutions that come to mind for fixing this are to either add support for overriding deal txs, the same as bids, or better yet: to handle deals from a completely separate account to avoid blocking on deals which are less time sensitive than auction bids. Any account can deal a finalised auction, so no proxy schemes are required for this.
Recent commit 80286b2 already adds support for setting deal tx gas prices dynamically, but I don't think this is is sufficient since deals still can't be overriden if the initial gas price chosen turned out to be too low.
I actually think c551a38 fixes this problem correctly when it comes to deal, as you are sending a transaction referring to a dynamic gas price strategy with gas_price=self.gas_price.
Having said that, this code still has six calls to .transact() with no gas price set explicitly, which will default to the gas price suggested by the node and never override these txs. You may be interested in reviewing them at some point as well.
I created a simple patch to disable deal here #46 but not sure about any side effects
@reverendus is right that this recent commit actually does allow deals to be overridden too. I would nevertheless consider having a separate deal keeper operating from a distinct EOA, with an optional argument to only deal auctions that were won by specified bidder(s), to completely disentangle the two queues (one of which is timestamp sensitive and the other not), reducing risk. @grandizzy Then your flag for disabling deal can be combined with running that deal keeper separately
I'm aware of this. Having a separate queue would only be helpful if the keeper was dealing using another account, which would complicate configuration. Disabling the deal (and handling it after congestion dies down) is the simplest solution. I will look into the difficulty of a replace mechanism.
I'm about to submit a PR which provides a mode where the keeper only kicks. This "kick keeper" could be extended to handle deals for a list of other accounts, which I'll look into next.