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

NUT for DLC execution #128

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

NUT for DLC execution #128

wants to merge 21 commits into from

Conversation

conduition
Copy link

@conduition conduition commented May 27, 2024

Depends on #127

Closes #122

Adds a NUT which enables DLC execution using a cashu mint as a blind intermediary. Based on this proposal.

dlc.md Outdated
Comment on lines 32 to 36
```python
Ki_ = Ki + bi * G
```

...for some blinding secret `bi` known to all DLC participants. Each locking point SHOULD be allocated a _unique_ blinding secret.
Copy link
Author

@conduition conduition May 27, 2024

Choose a reason for hiding this comment

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

I think it should be safe to use a single blinding secret b to mask every locking point, like this

Ki_ = b * Ki

This would be easier on the client, as the wallet only needs to store one blinding secret per DLC instead of n secrets. We would not be exposing any additional information to the mint, because the mint only ever sees at most one blinded locking point, and thus could not use the common factor of b for any trickery.

Can somebody check me on this?

dlc.md Outdated

For each `Payout` object, the mint performs the following checks:

1. Validate that `Payout.signature` is a valid [BIP-340] signature made by `Payout.pubkey` on `payout.dlc_root`
Copy link
Author

Choose a reason for hiding this comment

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

This could also be a hash preimage, or the dlog of Payout.pubkey.

@conduition conduition mentioned this pull request Jun 4, 2024
11 tasks
Not all clients will want to support BIP340 signatures,
so we should offer a way for them to claim a DLC by
simply exposing a secret key to the mint.
"dlc_root": "2db63c93043ab646836b38292ed4fcf209ba68307427a4b2a8621e8b1daeb8ed",
"outcome": {
"k": "8e935aec5668312be8f960a5ecc3c5dd290e39985970bfd093047df7f05cc9ec",
"P": "{\"361cd8bd1329fea797a6add1cf1990ffcf2270ceb9fc81eeee0e8e9c1bd0cdf5\":\"10000\"}"

Choose a reason for hiding this comment

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

shouldn't the value here be 1 as the lone payout gets the entirety of the funding amount?

Copy link
Author

Choose a reason for hiding this comment

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

It can be any positive integer. Since payout values are computed by relative weights, this number can be anything if there is only one recipient in the payout weights map.

"dlc_root": "2db63c93043ab646836b38292ed4fcf209ba68307427a4b2a8621e8b1daeb8ed",
"outcome": {
"timeout": 1716777419,
"P": "{\"361cd8bd1329fea797a6add1cf1990ffcf2270ceb9fc81eeee0e8e9c1bd0cdf5\":\"10000\"}"

Choose a reason for hiding this comment

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

Also here

"registrations": [
{
"dlc_root": "2db63c93043ab646836b38292ed4fcf209ba68307427a4b2a8621e8b1daeb8ed",
"funding_amount": <int>,

Choose a reason for hiding this comment

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

So all the provided inputs must be signed by keys of the same denomination? Or is just funding_amount in sats?

Copy link
Author

Choose a reason for hiding this comment

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

input proofs can be in any combination of denominations, as per the usual rules of swap/melt operations.

The new thing here is the funding_amount which is an explicit indicator to the mint, saying "i want to fund this DLC with exactly this much money". The value of all the input proofs will need to meet that threshold - or more if the mint charges fees. (see L254 for how fees are computed)

Note also that the threshold tag in the DLC secret is compared against the funding_amount

Copy link

@lollerfirst lollerfirst Jul 19, 2024

Choose a reason for hiding this comment

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

Yes I understand this, but how do we know what denomination the funding amount is in? I, for now, just assumed it is always sats but maybe it should be specified in each registration as an additional field

Copy link
Author

Choose a reason for hiding this comment

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

Oh sorry, I see what you mean. I forgot about multi-currency support. Yes, good idea, I added a unit field to disambiguate that for the mint. This is also something the mint needs to store (probably easier to just have different tables for DLCs funded under different units than to concretely store the unit as a string)

@conduition
Copy link
Author

Note to self todo:

  • Remove the atomic flag. It adds an unnecessary database-rollback requirement to the implementation and realistically very few users will need to register or resolve multiple DLCs atomically.
  • Instead of a single dedicated key for funding proof signatures, use the first key from the relevant unit's active keyset

dlc.md Outdated Show resolved Hide resolved
Comment on lines +392 to +396
```python
leaf_hash = SHA256(Settlement.outcome.k * G || Settlement.outcome.P)
assert merkle_verify(dlc_root, Settlement.merkle_proof, leaf_hash)
```

Choose a reason for hiding this comment

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

see my other comment on the timeout redemption

Remove the atomic flags for client requests. It creates
too much implementation complexity for too little use-case.
@conduition
Copy link
Author

conduition commented Aug 4, 2024

Added the following spec changes:

  • No more 'atomic' flags ce22a20
  • Timeout timestamps are now encoded as uint64 instead of uint32 b67e62a
  • Add recommendations for clients to avoid being scammed by replay attacks 6990d59
  • Improve security of the funding_proof signature by the mint. It now commits to the funding_amount explicitly, and to the funding unit implicitly, by using a signing key from an active keyset instead of a dedicated key. 95f47ba a86a4e8

1. If `Payout.dlc_root` does not correspond to any known funded DLC, return an error.
1. If `Payout.dlc_root` corresponds to a known DLC, but that DLC has not been settled, return an error.
1. If `Payout.pubkey` is not a key in the `debts` map, return an error.
1. If `sum([out.amount for out in Payout.outputs]) != debts[Payout.pubkey]`, return an error.
Copy link

@lollerfirst lollerfirst Sep 14, 2024

Choose a reason for hiding this comment

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

@conduition I think here it should be sum([out.amount for out in Payout.outputs]) != eligible_amount where eligible_amount is calculated as follows:

denom = sum(debts.values())
nom = debts[payout.pubkey]
eligible_amount = int(nom / denom * funding_amount)

Also, I was thinking we can discriminate based on whether the requested amount is equal or less than the eligible amount:

  • if it's equal, we drop the pubkey entry from the debts map
  • if it's less, we adjust the debts map so that the pubkey entry displays a weight value calculated such that new_eligible_amount == old_eligible_amount - requested_amount

Copy link
Author

@conduition conduition Sep 14, 2024

Choose a reason for hiding this comment

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

I think here it should be sum([out.amount for out in Payout.outputs]) != eligible_amount where eligible_amount is calculated as follows:

denom = sum(debts.values())
nom = debts[payout.pubkey]
eligible_amount = int(nom / denom * funding_amount)

Earlier in the document, we define debts as follows:

weights = json.loads(Settlement.outcome.P)
weight_sum = sum(weights.values())
debts = dict(((pubkey, funding_amount * weight // weight_sum) for pubkey, weight in weights.items()))

You can view debts as a mapping from weights[pubkey] to weights[pubkey] / sum(weights.values()) * funding_amount. Because sum((x / sum(set) for x in set)) == 1 for any set, we can assert:

sum(debts.values()) ~= funding_amount

(approximately, due to integer division rounding)

In your suggestion you have:

eligible_amount = int(debts[payout.pubkey] / sum(debts.values()) * funding_amount)

But as i just showed, sum(debts.values()) is approximately equivalent to funding_amount. Simplify your definition of eligible_amount and you'll find it's the same as debts[payout.pubkey]:

debts[payout.pubkey] / sum(debts.values()) * funding_amount
debts[payout.pubkey] / funding_amount * funding_amount
debts[payout.pubkey]

Copy link
Author

Choose a reason for hiding this comment

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

Also, I was thinking we can discriminate based on whether the requested amount is equal or less than the eligible amount:

Interesting. Is there any reason (other than a computational error) why a wallet wouldn't want to claim its full payout at once?

Copy link

@lollerfirst lollerfirst Sep 14, 2024

Choose a reason for hiding this comment

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

Aw, man. I think I might have misinterpreted/read too quickly that part of the spec. I thought the debts map was just the Settlement.outcome.P once it was validated (BTW that's also what I have implemented).

Any reason why it can't be?

[EDIT: forget it. much simplier your way]

Choose a reason for hiding this comment

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

Also, I was thinking we can discriminate based on whether the requested amount is equal or less than the eligible amount:

Interesting. Is there any reason (other than a computational error) why a wallet wouldn't want to claim its full payout at once?

It's just something that can happen. Do we let the mint just rug the claimant in that case? I thought it might be better not to.

Copy link
Author

@conduition conduition Sep 15, 2024

Choose a reason for hiding this comment

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

Any reason why it can't be?

Technically you could, but you'd also have to save the weight_sum. It's simpler to calculate the debts at settlement time rather than at claim time while you still have access to the full weights map.

It's just something that can happen. Do we let the mint just rug the claimant in that case? I thought it might be better not to.

If the claimant's client is buggy for some reason and won't compute the correct debt amount on its own, then a claimant could just use GET /v1/dlc/status/{dlc_root} to see the correct debts[pubkey] value and withdraw directly using a different client. But integer arithmetic is deterministic regardless of programming language, so this shouldn't happen as long as clients follow the spec.

As for specifically why i'm trying to avoid partial claims, it's because it would allow one more vector for clients to DoS the mint (by consuming space with pubkeys for cheap). This is something I was hoping to clean up once we had a working implementation we can test against.

Choose a reason for hiding this comment

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

This is something I was hoping to clean up once we had a working implementation we can test against.

Right now it should do everything. It surely has a million problems but you can test it if you want.

Copy link

@lollerfirst lollerfirst Sep 16, 2024

Choose a reason for hiding this comment

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

As for specifically why i'm trying to avoid partial claims, it's because it would allow one more vector for clients to DoS the mint (by consuming space with pubkeys for cheap). This is something I was hoping to clean up once we had a working implementation we can test against.

Right now if the DLC is overfunded, the mint automatically adjusts the funding amount. So one client might unknowingly provide an amount which doesn't exactly match their eligible amount, thereby failing the transaction.

Copy link
Author

Choose a reason for hiding this comment

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

So one client might unknowingly provide an amount which doesn't exactly match their eligible amount, thereby failing the transaction.

If a DLC is overfunded, all clients involved in the DLC must know about it. The funding_amount is committed to in the DlcFundingProof that the funder uses to prove DLC registration. If for some reason a funder goes silent after registering and overfunding a DLC, their peer clients can simply use GET /v1/dlc/status/{dlc_root} to see the true funding amount. This endpoint will also explicitly return the debts map once the DLC is settled, allowing faulty clients to recover gracefully if the POST /v1/dlc/payout request fails due to a mismatching output sum.

I just don't see the point in allowing partial payouts when all these options are available. It feels like an invitation to a class of "missing money" bugs in client implementations where clients withdraw some money and leave the rest on the table unknowingly. It also allows one more way to DoS the mint which we'll have to clean up later.

Copy link
Author

Choose a reason for hiding this comment

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

it's your call if you'd like to implement partial withdrawals (it's your code after all), but I would recommend against it.

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.

NUT for Discreet Log Contract support
2 participants