Skip to content

multi: Keep trying init with the same secret. #3269

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

JoeGruffins
Copy link
Member

@JoeGruffins JoeGruffins commented May 9, 2025

There was a problem reported with an init was sent twice. This was due to a nonce issue. This pr ignores that and attempts to prevent a second init being sent regardless. It currently doesn't work because I'm unsure how to get the txid of the swap and server wants that. I think @martonp said something about this being possible before, by examining the data from tx history, but unsure. Or we could just fail, which seems like a waste. Or change the server logic.

@dev-warrior777
Copy link
Contributor

I think @martonp said something about this being possible before, by examining the data from tx history, but unsure. Or we could just fail, which seems like a waste. Or change the server logic.

Data from tx history is not available always e.g. electrum. Am seeing some refund issues that happily seem to resolve.

@JoeGruffins
Copy link
Member Author

JoeGruffins commented May 9, 2025

For a quick fix, it looks like just failing and waiting for refund may be cleanest, but a waste and will hurt the rep of the user. I guess as a backup for nonce's getting in a twist it's preferable to the ghost init that cant easily be tracked down and requires numerous steps to refund.

@JoeGruffins JoeGruffins force-pushed the keepsecretwhenfail branch 2 times, most recently from 6a01b20 to f635d80 Compare May 10, 2025 02:51
@JoeGruffins JoeGruffins marked this pull request as ready for review May 10, 2025 02:58
@JoeGruffins
Copy link
Member Author

JoeGruffins commented May 10, 2025

This is alot of extra things to do, but note that it only triggers if the first attempt at init failed.

@JoeGruffins
Copy link
Member Author

JoeGruffins commented May 10, 2025

Also contains marton fixes from #3082

Maybe one of them makes the nonce issue better anyway.

@JoeGruffins
Copy link
Member Author

This is alot of extra things to do, but note that it only triggers if the first attempt at init failed.

srry, I guess the check also triggers first try if taker.

@JoeGruffins JoeGruffins force-pushed the keepsecretwhenfail branch from f635d80 to 3e57287 Compare May 10, 2025 23:17
Comment on lines +1769 to +1774
// InitChecker is an account wallet that checks the status of a swap init.
type InitChecker interface {
// AlreadyInitialized returns if the swap was already initialized and
// some data needed to interact with the contract later.
AlreadyInitialized(assetVersion uint32, contract *Contract) (initialized bool, zeroHash, contractData []byte, err error)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could Swap just check whether or not the swap is already initialized, the same way how we check isRedeemable and isRefundable, and if it's already initialized then return without error as if the transaction was just sent. Then this interface and the checkInitialized function in trade.go would not be needed. The server just needs to be updated to not require the transaction hash.

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 didn't want to update the server as thats a bigger change. But we can do that. May not have time to work on this until week after next.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll take a look.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is quite a big change.. I think let's just go with revoking for now.

@JoeGruffins JoeGruffins requested a review from buck54321 May 29, 2025 02:01
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.

3 participants