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

Allow selection of multiple utxos in pad_alignment_output and add_value #1858

Closed
wants to merge 3 commits into from

Conversation

gmart7t2
Copy link
Contributor

ord wallet inscribe only adds at most 1 padding alignment utxo and 1 value utxo.

If you try to inscribe a tiny file with 5 cardinal outputs of size 4k sats each in your wallet the inscribe will fail because ord isn't able to use enough inputs to make up the 10k postage.

This commit fixes that.

An example:

We have five 4k outputs:

$ ord wallet outputs | jq -cM .[]
{"output":"adea31b810f76ff7ff92332e1260c422b9326cf66f45c24451e613f01fff5009:1","amount":4000}
{"output":"eb9f7b9427062d672118c2882df235c489208a314ed8a7f45f8b0709bc226514:1","amount":4000}
{"output":"e1e2b82a40f33ee968c5ffbda541dc7a9d67b916da717e36065b7a92f5d0965e:0","amount":4000}
{"output":"fb2e602809d77569623ae348f17bc593515003181bc3d81db97d0f9c0b0b2c79:0","amount":4000}
{"output":"fc61bc0be498e348b76503bbbab06fc3e0bcc5bf799ebdec847693143f582baf:1","amount":4000}

ord can't inscribe onto any of them:

$ ord wallet inscribe --fee-rate 1 /tmp/hello.txt
error: wallet does not contain enough cardinal UTXOs, please add additional funds to wallet.

But with this pull request it can:

$ ord wallet inscribe --fee-rate 1 /tmp/hello.txt
{
  "commit": "dd49221d3ad3b02c0a9671c014974191b43d2b265805641dda908e74d1fe06da",
  "inscription": "5e1807eaca2267c6273bca0ed13b5ad280efcd474def7b8ea189b8d2d2e121dbi0",
  "reveal": "5e1807eaca2267c6273bca0ed13b5ad280efcd474def7b8ea189b8d2d2e121db",
  "fees": 408
}

Here are the commit tx inputs:

$ bitcoin-cli getrawtransaction dd49221d3ad3b02c0a9671c014974191b43d2b265805641dda908e74d1fe06da 1 | jq -cM '.vin[]|{txid,vout}'
{"txid":"adea31b810f76ff7ff92332e1260c422b9326cf66f45c24451e613f01fff5009","vout":1}
{"txid":"eb9f7b9427062d672118c2882df235c489208a314ed8a7f45f8b0709bc226514","vout":1}
{"txid":"e1e2b82a40f33ee968c5ffbda541dc7a9d67b916da717e36065b7a92f5d0965e","vout":0}

And outputs:

$ bitcoin-cli getrawtransaction dd49221d3ad3b02c0a9671c014974191b43d2b265805641dda908e74d1fe06da 1 | jq -cM '.vout[]|{address:.scriptPubKey.address,value:(.value*1e8|round)}'
{"address":"bcrt1p9c0a8d2e7zv3v78yvv2ppctnc0xdhtutjneh5vu6dwmqde49e7fqaxljnp","value":10139}
{"address":"bcrt1purf9fr8hqcznq8v4vkgdauu9396efuuv3tl5gaef6h7pqtzx7c7q42v2h7","value":1592}

Copy link
Collaborator

@raphjaph raphjaph left a comment

Choose a reason for hiding this comment

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

This is great!

Could you add a unit test (bottom of transaction_builder.rs) that tests the case you outline in your initial comment? If you can, add tests for the prefer_under flag. So that both cases are properly tested.

@gmart7t2 gmart7t2 force-pushed the select-multiple-utxos branch from 2e2631f to c34ce77 Compare March 8, 2023 15:45
@gmart7t2
Copy link
Contributor Author

gmart7t2 commented Mar 8, 2023

Of course. I added 3 new tests to test:

  • adding multiple utxos for value,
  • adding multiple utxos for padding,
  • and a separate test for the prefer_under flag.

@gmart7t2 gmart7t2 force-pushed the select-multiple-utxos branch from f56ab03 to 045731d Compare March 8, 2023 16:08
@gmart7t2 gmart7t2 requested a review from raphjaph March 8, 2023 22:53
cryptonaut420 added a commit to cryptonaut420/ord that referenced this pull request Mar 12, 2023
@BTCAlchemist
Copy link

Many people in the Ordicord Discord server have encountered the error, error: wallet does not contain enough cardinal UTXOs, please add additional funds to wallet, so a fix for this would be helpful. At present, before inscribing, users need to send a separate UTXO to their ord wallet with sufficient sats based on a fee estimate. If ord wallet were to natively consolidate sats from non-inscription UTXOs to use for inscriptions, this would save users time and money from creating shell UTXOs for inscriptions and would not require users to calculate exact fee estimations for the shell UTXOs.

In addition, there may be extra UTXO sats post-inscription because fee estimates may not be precise. This leads to tons of dust UTXOs that need to be consolidated and consume transaction fees.

@raphjaph Appreciate you reviewing this. Do the changes from @gmart7t2 that you requested look good?

@ordinalsbot
Copy link

this is definitely helpful but I want to point out a potential issue with combining multiple inputs. the fee could get real expensive in these situations. is it possible to put a max-total-fee limit to this command so that you can still have control over the total fees paid for a certain inscription?

@gmart7t2
Copy link
Contributor Author

put a max-total-fee limit to this command so that you can still have control over the total fees paid for a certain inscription?

Would --max-fee or --max-inputs be better? The fee depends on the file's size and the fee rate, so perhaps it is better to have the new flag limit the total number of inputs to the commit tx.

The utxo selection code will use the biggest available cardinals in an attempt to minimize the number it uses but you're right that in the worst case it could use hundreds of low value utxos that barely cover their own fee cost.

What do you think?

@gmart7t2 gmart7t2 force-pushed the select-multiple-utxos branch from f891f99 to 045731d Compare April 12, 2023 14:50
@tyjvazum
Copy link

@gmart7t2, your expertise would be very appreciated in arb if you'd like to contribute to it.

Copy link
Collaborator

@raphjaph raphjaph left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good! I can't push changes to this branch though. I might have to open a new PR with these changes and merge there. #2303

@raphjaph raphjaph mentioned this pull request Jul 24, 2023
@raphjaph
Copy link
Collaborator

Merged #2303 which is duplicate of this one. So closing this.

@raphjaph raphjaph closed this Jul 30, 2023
Copy link

@tawhidnazari57 tawhidnazari57 left a comment

Choose a reason for hiding this comment

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

Binance Web3

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.

6 participants