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

Use get_transfer_by_txid to extract the exact block height of txlock #121

Closed
wants to merge 2 commits into from

Conversation

da-kami
Copy link
Member

@da-kami da-kami commented Jan 6, 2021

This is just for reference. If you spot what the problem could be we could still go for this, otherwise I would opt for the solution proposed by #119
This fails with transaction not found on stagenet, not sure why.

…x_lock

This failed on stagenet because somehow the transaction cannot be found with the given tx_hash.
This might be because tx are not indexed or other problem, hard to say at this point.
@da-kami da-kami changed the title WIP - use get_transfer_by_txid to extract the exact block height of t… Use get_transfer_by_txid to extract the exact block height of txlock Jan 6, 2021
@D4nte
Copy link
Contributor

D4nte commented Jan 7, 2021

This is just for reference. If you spot what the problem could be we could still go for this, otherwise I would opt for the solution proposed by #119
This fails with transaction not found on stagenet, not sure why.

Is it fine in regtest?

@da-kami
Copy link
Member Author

da-kami commented Jan 7, 2021

This is just for reference. If you spot what the problem could be we could still go for this, otherwise I would opt for the solution proposed by #119
This fails with transaction not found on stagenet, not sure why.

Is it fine in regtest?

Happy path test failed on CI, so there is definitely a problem. It might be time consuming to pin it down.

@D4nte
Copy link
Contributor

D4nte commented Jan 8, 2021

get_transfer_by_txid only work if the transfer is to or from the current wallet.

@da-kami
Copy link
Member Author

da-kami commented Jan 8, 2021

get_transfer_by_txid only work if the transfer is to or from the current wallet.

In my understanding the transfer is to Bob's wallet so it should work. But true, we are creating this wallet later, so it might not be recorded as transaction to said wallet. I wonder how we can extract the height from the tx then.

@D4nte
Copy link
Contributor

D4nte commented Jan 8, 2021

get_transfer_by_txid only work if the transfer is to or from the current wallet.

In my understanding the transfer is to Bob's wallet so it should work. But true, we are creating this wallet later, so it might not be recorded as transaction to said wallet. I wonder how we can extract the height from the tx then.

The transfer is not to Bob's wallet as the transfer is locked with keys that are revealed by the Bitcoin redeem/punish adaptor signatures.

I don't think we can extract the height but elementary maths should help.

@da-kami
Copy link
Member Author

da-kami commented Jan 8, 2021

get_transfer_by_txid only work if the transfer is to or from the current wallet.

In my understanding the transfer is to Bob's wallet so it should work. But true, we are creating this wallet later, so it might not be recorded as transaction to said wallet. I wonder how we can extract the height from the tx then.

The transfer is not to Bob's wallet as the transfer is locked with keys that are revealed by the Bitcoin redeem/punish adaptor signatures.

I don't think we can extract the height but elementary maths should help.

If we really cannot extract the exact height I would stick to the simple solution of #119 - this is bulletproof and does not cost much (scanning a few more blocks won't hurt). I would like to avoid further error scenarios.

@D4nte
Copy link
Contributor

D4nte commented Jan 11, 2021

get_transfer_by_txid only work if the transfer is to or from the current wallet.

In my understanding the transfer is to Bob's wallet so it should work. But true, we are creating this wallet later, so it might not be recorded as transaction to said wallet. I wonder how we can extract the height from the tx then.

The transfer is not to Bob's wallet as the transfer is locked with keys that are revealed by the Bitcoin redeem/punish adaptor signatures.
I don't think we can extract the height but elementary maths should help.

If we really cannot extract the exact height I would stick to the simple solution of #119 - this is bulletproof and does not cost much (scanning a few more blocks won't hurt). I would like to avoid further error scenarios.

I am happy for us to move forward with the solution implemented in #119 where Bob Remembers the block height after Bitcoin is locked and before he sends the encrypted signature (to redeem BTC) to Alice.

bors bot added a commit that referenced this pull request Jan 12, 2021
119: Remember the block-height before XMR lock for generated monero wallet r=da-kami a=da-kami

The first approach #121 was using `get_transfer_by_txid` that allows extrancting the exact tx-lock 1st confirmation block height. 
But that introduced an additional error scenario, and I actually ran into that error scenario (`transaction not found`) once I ran it on `stagenet`. Might be that `get_transfer_by_txid` requires running the node in a specific way (like `txindex` on bitcoin).  
I am not sure at this stage and don't want to invest more time.

Long story short: 
I opted for just recording the height before watching for XMR locked. This means that we record a height right after sending the Bitcoin lock tx. (Because we start watching for XMR lock right after that.) 
Bob's new wallet unnecessarily scans an additional 7+ blocks (assuming inclusion in the next Bitcoin block and one confirmation for Monero lock) every time which is a matter of milliseconds. Not worth optimising this further at this stage. This solution is more resilient as well, because it does not add another error scenario.

Co-authored-by: Daniel Karzel <daniel@comit.network>
@D4nte D4nte closed this Jan 13, 2021
@D4nte D4nte deleted the extract-tx-lock-inclusion-block-height branch January 13, 2021 03:40
abraham-nixon added a commit to abraham-nixon/xmr-btc-swap that referenced this pull request Feb 15, 2022
119: Remember the block-height before XMR lock for generated monero wallet r=da-kami a=da-kami

The first approach comit-network/xmr-btc-swap#121 was using `get_transfer_by_txid` that allows extrancting the exact tx-lock 1st confirmation block height. 
But that introduced an additional error scenario, and I actually ran into that error scenario (`transaction not found`) once I ran it on `stagenet`. Might be that `get_transfer_by_txid` requires running the node in a specific way (like `txindex` on bitcoin).  
I am not sure at this stage and don't want to invest more time.

Long story short: 
I opted for just recording the height before watching for XMR locked. This means that we record a height right after sending the Bitcoin lock tx. (Because we start watching for XMR lock right after that.) 
Bob's new wallet unnecessarily scans an additional 7+ blocks (assuming inclusion in the next Bitcoin block and one confirmation for Monero lock) every time which is a matter of milliseconds. Not worth optimising this further at this stage. This solution is more resilient as well, because it does not add another error scenario.

Co-authored-by: Daniel Karzel <daniel@comit.network>
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

2 participants