Implement dynamic amount of tokens for change #223
Conversation
With the recent update to NUT-08, we can ensure that the amount of blank outputs is always enough to cover any overpaid lightning fees. This change implements this functionality for both the wallet and the mint. The mint updateis backwards-compatible with respect to old wallets.
| def calculate_number_of_blank_outputs(fee_reserve_sat: int): | ||
| """Calculates the number of blank outputs used for returning overpaid fees. | ||
|
|
||
| The formula ensures that any overpaid fees can be represented by the blank outputs, | ||
| see NUT-08 for details. | ||
| """ | ||
| assert fee_reserve_sat > 0, "Fee reserve has to be positive." | ||
| return max(math.ceil(math.log2(fee_reserve_sat)), 1) |
There was a problem hiding this comment.
Not sure if I really should put it in here, or rather into the wallet directly.
| expected_returned_fees = 1900 | ||
|
|
||
| n_blank_outputs = calculate_number_of_blank_outputs(fee_reserve) | ||
| blinded_msgs = [step1_alice(str(n)) for n in range(n_blank_outputs)] |
There was a problem hiding this comment.
Should we rather use pre-computed blinded messages here? We don't use them for anything in this test, but maybe better to have it as deterministic as possible.
There was a problem hiding this comment.
You can use a predetermined blinding_factor when calling step1_alice, then the outcome is determinstic!
There was a problem hiding this comment.
Actually, now I'm thinking this might break legacy wallets which always send 4 outputs (or we add an exception to that but that would be extra ugly).
There was a problem hiding this comment.
You can use a predetermined blinding_factor when calling step1_alice, then the outcome is determinstic!
Yeah, that was my proposal. I guess that's a yes from your side, so I'll add predefined blinding factors. 👍
Actually, now I'm thinking this might break legacy wallets which always send 4 outputs (or we add an exception to that but that would be extra ugly).
Sorry, I'm a bit confused here, can you expand on this? How does it break legacy wallets? As you can see below, I also added a test for it.
There was a problem hiding this comment.
What I meant: Legacy wallets send 4 outputs, but if our new algo says "we only need 3" then legacy wallets would get an error. We can leave it as you did: just ignore outputs that are unnecessary.
I think it's safe to ignore this for now. There is more than just one place in the code where we can DoS the mint.
There was a problem hiding this comment.
What I meant: Legacy wallets send 4 outputs, but if our new algo says "we only need 3" then legacy wallets would get an error.
Ah, I think I get what you mean now. However, I think it was already the case before that less outputs could be sent back (e.g. if the return output fits into 3). See line 557 and following in the old version:
return_amounts = amount_split(user_paid_fee_sat - ln_fee_sat)
# we only need as many outputs as we have change to return
outputs = outputs[: len(return_amounts)]At least if I understand that part correctly. 🙂
|
I've added tests for the actual equation and the mint implementation. Let me know if we also should do something for the |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #223 +/- ##
==========================================
+ Coverage 55.42% 55.50% +0.08%
==========================================
Files 41 41
Lines 3284 3326 +42
==========================================
+ Hits 1820 1846 +26
- Misses 1464 1480 +16
☔ View full report in Codecov by Sentry. |
|
Excellent PR! Will review |
| if user_paid_fee_sat - ln_fee_sat > 0 and outputs is not None: | ||
| # we will only accept at maximum n_return_outputs outputs | ||
| assert len(outputs) <= n_return_outputs, Exception( | ||
| "too many change outputs provided" | ||
| ) |
There was a problem hiding this comment.
Although not strictly necessary, for DOS safety, we could add a check here using calculate_number_of_blank_outputs to check whether the user has generated too many blank outputs.
There was a problem hiding this comment.
Sure, can do. What do you want to happen in that case? Right now, if a wallet sends too many blank outputs, I'm just ignoring the one that are not necessary, see line 558.
| async def test_generate_change_promises_legacy_wallet(ledger: Ledger): | ||
| # Check if mint handles a legacy wallet implementation (always sends 4 blank | ||
| # outputs) as well. |
There was a problem hiding this comment.
Excellent! Very thoughtful!
|
LGTM^2 |
With the recent update to NUT-08, we can ensure that the amount of blank outputs is always enough to cover any overpaid lightning fees. This change implements this functionality for both the wallet and the mint. The mint updateis backwards-compatible with respect to old wallets.
Closes #183