Skip to content
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

eth: refactor tx montoring, handle tx resubmissions better, general refactoring #2752

Merged
merged 12 commits into from
May 22, 2024

Conversation

buck54321
Copy link
Member

This work accomplishes a number of goals required for putting eth and polygon into production.

  • Fix nonce tracking and pending tx monitoring. Some bugs came out in testnet testing and one of them put us in a state where we were rebroadcasing txs with a nonce too low and they weren't being accepted.
  • Implement for eth/polygon new rule that we will not automatically generate replacement txs if the tx being replaced incurred tx fees. New system to getting user authorization in these hopefully exceedingly rare edge cases.
  • Add some caches and fix some spots where we were hitting the rpc way too much. Use our DB and tx monitoring for rpc-free info.
  • A bunch of general refactoring.
  • One unrelated UI fix snuck in regarding 0-lot max fee result messaging. Sorry.

Hand-tested. Will do some self-review asap.

@buck54321
Copy link
Member Author

Not ready for testing quite yet.

@buck54321 buck54321 marked this pull request as ready for review May 2, 2024 03:47
@buck54321
Copy link
Member Author

It's ready again.

@buck54321 buck54321 force-pushed the ethrefactor branch 3 times, most recently from d6973fa to 7b8ba91 Compare May 5, 2024 12:18
Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow. Let's get this merged.

client/asset/eth/contractor.go Outdated Show resolved Hide resolved
client/asset/eth/eth.go Outdated Show resolved Hide resolved
client/asset/eth/eth.go Outdated Show resolved Hide resolved
client/asset/eth/eth.go Outdated Show resolved Hide resolved
client/asset/eth/eth.go Outdated Show resolved Hide resolved
client/asset/eth/eth.go Outdated Show resolved Hide resolved
client/asset/eth/eth.go Outdated Show resolved Hide resolved
client/asset/eth/eth.go Outdated Show resolved Hide resolved
client/asset/eth/eth.go Outdated Show resolved Hide resolved
client/asset/eth/txdb.go Outdated Show resolved Hide resolved
Copy link
Contributor

@martonp martonp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested the nonce replacement, worked well. The nonce on the UI is undefined though. Also, the message could say "A different transaction with the same nonce was confirmed first."

Screenshot 2024-05-11 at 3 12 56 PM

client/asset/eth/txdb.go Show resolved Hide resolved
client/core/core.go Show resolved Hide resolved
client/core/notification.go Outdated Show resolved Hide resolved
client/asset/eth/eth.go Outdated Show resolved Hide resolved
client/asset/eth/eth.go Show resolved Hide resolved
client/asset/eth/eth.go Outdated Show resolved Hide resolved
client/asset/eth/txdb.go Outdated Show resolved Hide resolved
client/asset/eth/eth.go Outdated Show resolved Hide resolved
client/core/trade.go Outdated Show resolved Hide resolved
Comment on lines 3022 to 3028
case errors.Is(err, asset.ErrTxLost):
// The transaction was nonce-replaced or otherwise lost without
// rejection or with user acknowlegement. Try again if we're the taker.
// If we're the maker we'll give up and try to refund.
c.log.Infof("Redemption %s (%s) has been noted as lost.", match.MetaData.Proof.TakerRedeem, unbip(toWallet.AssetID))
if match.Side == order.Taker {
match.MetaData.Proof.TakerRedeem = nil
match.Status = order.MakerRedeemed
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case the counterparty of the maker could have also got the secret. You didn't set match.MetaData.Proof.SelfRevoked = true over there though.. so will it refund?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added commented tests for all actions now.

Copy link
Contributor

@martonp martonp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I overrode the nonce and did not provide a replacement transaction, it will stay like this in pending forever. Maybe we could put a place to input the replacement transaction on the tx details popup. Could be in another PR though.

Screenshot 2024-05-19 at 9 18 02 AM

client/core/trade.go Outdated Show resolved Hide resolved
client/asset/eth/eth.go Outdated Show resolved Hide resolved
client/asset/eth/eth.go Outdated Show resolved Hide resolved
client/asset/eth/eth.go Show resolved Hide resolved
client/asset/eth/eth.go Outdated Show resolved Hide resolved
@buck54321 buck54321 merged commit 93e7e87 into decred:master May 22, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants