Check burning man receiver address validity #7203
Merged
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
Avoid streaming over the entire proposals list to find a matching txId, for every 'Issuance' & 'CompensationProposal' pair used to construct and add a compensation model to the burn output model of each candidate. Instead, stream over the proposals list once, doing lookups by txId of each matching issuance, which uses the TreeMap 'DaoState.issuanceMap', thereby taking O(n*log(n)) time.
This avoids needless hashing & equality comparisons of instances of 'BurningManCandidate', which are quite large mutable objects (so should probably use reference equality anyway, and not be used as keys). Also rearrange a couple of (package) private methods.
Handle the exceptional case of a receiver address chosen from a cycle where the candidate somehow got more than one compensation proposal accepted. Either the last custom address or first issuance change address is supposed to be chosen for the receiver address, but in case of a tie at that vote result height, take the address that comes first in lexicographic order.
These are both redundant now and will always return true. Also add a missing past check for Proposal 412 activation to 'RefundManager', instead of just defaulting to the current date, in case of any very old disputes involving DPTs created prior to the activation.
Set the burn cap of a candidate to zero if he has an invalid receiver
address, that is, one that bitcoinj cannot parse. This prevents trade
failure when creating the DPT, by making such BM inactive and
distributing their share to the other BM. (Setting the burn cap to zero
is a little more robust than simply filtering out such candidates, as
'BurningManService' handles subsequent share redistribution better than
'(DelayedPayoutTx|BtcFee)ReceiverService'.)
While this case should normally never occur, due to UI validation of the
custom receiver address, there are at least two ways a BM could
invalidate his own receiver address if so inclined:
1) He could simply bypass the UI validation;
2) He could manually create a compensation issuance tx with a change
address type unrecognised by bitcoinj, such as P2TR, as the address
field is pulled straight from the RPC JSON by each full DAO node.
Thus, it is necessary to check both change and custom addresses.
Reject any custom receiver address which wasn't encoded as Bech32m if the witness program version is greater than zero. These are currently accepted by bitcoinj but are now invalid and would fail to parse if our fork was updated to understand Bech32m, to support sending to P2TR addresses, which the upstream version appears to. (Thus, the presence of such malformed receivers would not be an issue at present, but might cause complications in the future.)
|
There is another small change to I noticed that bitcoinj allows any witness v1-v16 program (up to length 40 bytes) to be decoded from a Bech32 address, not just witness v0 (that is, p2wpkh & p2wsh). These give standard txs when used as outputs (and it doesn't look like bitcoinj would fail to validate or broadcast such txs either), though all of them apart from p2tr ScriptPubKeys would be anyone-can-spend and couldn't be spent in standard txs. However, since Taproot was introduced, witness v1 programs and above are supposed to be encoded as Bech32m, not Bech32, so really any Segwit address that isn't witness v0 shouldn't validate unless/until our fork of bitcoinj parses them correctly as Bech32m (which is already done in the upstream version). Otherwise, there could be complications making such an upgrade. |
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.
Set the burn cap of a candidate to zero if he has an invalid receiver address, that is, one that bitcoinj cannot parse. This prevents trade protocol failure when creating the DPT, by making such BM inactive and distributing their shares to the other BM. (Setting the burn cap to zero is a little more robust than simply filtering out such candidates, as
BurningManServicehandles subsequent share redistribution better thanDelayedPayoutTxReceiverServiceorBtcFeeReceiverService.While this case should normally never occur, due to UI validation of the custom receiver address, there are at least two ways a BM could invalidate his own receiver address if so inclined:
Thus, it is necessary to check both change and custom addresses.
--
This PR additionally does a couple of minor optimisations to
BurningManService, fixes nondeterministic receiver address selection in the edge case of multiple accepted compensation proposals by the same candidate in the same cycle, and cleans up redundant date checks for the activation of fixes for #6699 and bisq-network/proposals#412.None of the changes should alter the candidate burn shares or receiver addresses, except in possible future edge cases where the addresses would be nondeterministic or invalid, resulting in DPT creation/validation errors anyway (and thus failure to open the trade). So an activation date for these changes shouldn't be necessary.