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 fundrawtransaction more in the suggested ANT workflow #473

Merged
merged 1 commit into from
Oct 18, 2021

Conversation

domob1812
Copy link

Upstream bitcoin/bitcoin#17211 solved a long-standing issue for doing atomic trades with names (namely that the buyer's wallet was not able to directly fund a transaction with the non-wallet name input in it).

Based on this, we can now streamline name_ant_workflow.py to make use of this new feature. This allows us to properly use fundrawtransaction to fund the final transaction including already the name input and output, and thus with correct fee estimation done. (Previously we just funded a dummy transaction with a guessed higher fee, and then added in the name input and output manually, hoping the fee would still be fine.)

@domob1812 domob1812 added this to the nc0.23 milestone Oct 11, 2021
@domob1812
Copy link
Author

domob1812 commented Oct 11, 2021

@JeremyRand What do you think about this, is this how we want to "suggest" ANTs to be done? The main drawback I see here is that the solver in fundrawtransaction requires the pubkey of the payment address, rather than just the address - so this is a bit more information that the seller needs to pass to the buyer (address and pubkey).

Overall, though, this seems like a clear improvement (and in fact the Bitcoin PR merged fixes an issue I believe we discussed and brought up a long time ago in this context already).

@yanmaani
Copy link

There's an RPC method in the pipeline for this which will make the manual method obsolete anyway. I will send in a PR once bitcoin/bitcoin#23201 is merged. That said, I don't imagine this code will be going away any time soon, so it seems reasonable enough to keep it updated.

Concept ACK b59aa1c.

@domob1812
Copy link
Author

There's an RPC method in the pipeline for this which will make the manual method obsolete anyway. I will send in a PR once bitcoin/bitcoin#23201 is merged. That said, I don't imagine this code will be going away any time soon, so it seems reasonable enough to keep it updated.

Concept ACK b59aa1c.

Thanks for the review and the pointer to the other upstream PR, this will indeed be useful as well.

I still think that even if there is an RPC implementing the flow, it is good for us to have this test, both to ensure that the workflow actually works (although any regtest for the new RPC method will test this as well of course), and also more as a kind of documentation for how it works - perhaps for certain specific applications the high-level RPC method will not be suitable directly, and then users will have to fall back to a manual process.

Upstream bitcoin/bitcoin#17211 solved a
long-standing issue for doing atomic trades with names (namely that
the buyer's wallet was not able to directly fund a transaction with the
non-wallet name input in it).

Based on this, we can now streamline name_ant_workflow.py to make use
of this new feature.  This allows us to properly use fundrawtransaction
to fund the final transaction including already the name input and output,
and thus with correct fee estimation done.  (Previously we just funded a
dummy transaction with a guessed higher fee, and then added in the name
input and output manually, hoping the fee would still be fine.)
@domob1812 domob1812 merged commit 9e0680a into namecoin:master Oct 18, 2021
@domob1812 domob1812 deleted the ant-update branch October 18, 2021 04:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants