wallet: don't clear out all the blinding data when dropping change #1172
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
apoelstra
force-pushed
the
2022-09--wallet-fix
branch
2 times, most recently
from
d110a79
to
982ed60
Compare
|
ack 982ed60 - passes our internal CI tests now, thanks |
|
ACK 6259809 I ran the regression test locally: |
delta1
approved these changes
Sep 20, 2022
|
Holding off on merge because I think @psgreco would like to forward-port the existing elements-22 branch so that I can rebase on that. Meanwhile @jgriffiths believes there is another related bug related to transactions that carry issuances. I'll track that down and open a new PR on top of this one. |
When the fee is not subtracted from the outputs, the amount that has been reserved for the fee (change_and_fee - change_amount) must be enough to cover the fee that is needed. It would be a bug to not do so, so use an assert to make this obvious if such a situation were to occur. Github-Pull: bitcoin/bitcoin#22686 Rebased-From: d926232
…hat doesn't make sense with assets
First, this reverts commit ca2d72a to reinstate an assertion that was added in Bitcoin #22686. It did not compile because our `change_and_fee` variable is a map rather than number; I changed it to use `map_change_and_fee.at(policyAsset)` to match the equivalent change 2 lines down from a5d97b3 (merge of Bitcoin #22008). Then fix the following bugs: 1. Change the new test in rpc_fundrawtransaction.py to bump the -maxtxfee value, which we'd otherwise exceed, failing the test and masking actual failures. (This was just caused by the extreme fee settings of the test combined with Elements' large transactions.) 2. Change the fee-output size estimation for `tx_noinputs_size` to be 46 rather than 44 bytes; we forgot that even null surjection/rangeproofs need a 0 byte when output witnesses are present. This mistake triggered the new assertion. 3. Correct the logic in which change outputs are sometimes dropped even when they are the only blinded output in a transaction with blinded inputs. This would cause the new test to fail with `bad-txn-inputs-ne-outputs`; I'm very surprised that no existing tests hit this. (I have an existing comment block in this code where I "promise" that I had a good reason for doing something mysterious related to blinding. I was not able to reverse-engineer my intention here, though I think it is related to this, but since I couldn't understand it I just left this block intact and worked around it.) 4. This then triggered the assertion again since the coin selection code assumes that sufficiently-small change will always be dropped. If we prevent this drop we will have under-funded the transaction. To fix this we add Yet Another Flag `may_need_blinded_dummy` in which we add extra weight to `tx_noinputs_size` in the case that we're doing a blinded tx but have no blind destinations. We turn this off after coin selection if it turns out that we don't have any blinded inputs, though ofc at that point much of the damage/inefficiency has already been done.. 5. Fix some constants in other functional tests which assumed precise fee calculations; these precise values changed because of fixes (2) and (4). There is one new FIXME, which is that the "dummy change" value will now be a zero-valued OP_RETURN but we still put a full-size rangeproof and surjection proof on it. There is some plausible privacy benefit to this but not much, and wasting 5000+ bytes rather than the ~65 needed for an exact-value proof is not worth it. We will fix this in the future when we overhaul the wallet blinding logic.
The Elements 22 blinding logic has an edge case where when we drop change, leaving only a single blinded output, we recompute a bunch of blinding data to handle the potential for us to have 0 inputs and 1 output to blind. (BlindTransaction will fail in this case because it cannot make the transaction balance with only one output to mess with.) In this recomputation, we dropped more data than we meant to, causing us to incorrectly blind an output.
apoelstra
force-pushed
the
2022-09--wallet-fix
branch
2 times, most recently
from
bed83af
to
3c896b1
Compare
|
Rebased on the spend.cpp-relevant Bitcoin backports already in Elements 22, so we should be able to merge this into the 22 branch without too much trouble. |
Prior to coin selection we need to indicate that the issuances will take extra space, otherwise we may fail to select enough coins to cover our fees, triggering the new "fee needed exceeds fees available" assertion.
|
tested ack e5e3ec2 With this PR our internal CI tests now pass. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The Elements 22 blinding logic has an edge case where when we drop change, leaving only a single blinded output, we recompute a bunch of blinding data to handle the potential for us to have 0 inputs and 1 output to blind. (BlindTransaction will fail in this case because it cannot make the transaction balance with only one output to mess with.)
In this recomputation, we dropped more data than we meant to, causing us to incorrectly blind an output.