Make sure to use the segwit version of Script.correctlySpends in
TradeWalletService.finalizeDelayedPayoutTx, which requires the input
value and witness to be passed explicitly (as the latter holds the
actual signature). This was causing BuyerFinalizesDelayedPayoutTx to
fail to do any kind of signature check.
Also refactor the method slightly and remove a redundant call to
WalletService.checkScriptSig (which does the same thing as
TransactionInput.verify) in the branch used by the seller.
Disallow non-P2WH depositTx inputs from the taker, while continuing to
allow them from the maker, so that offers created pre-v1.5.0 can still
be taken. (After some time, those inputs could be disallowed too.)
This is mainly to prevent mass blackmail attacks, where more victims'
money could be locked up than the DAO could possibly compensate them all
for. (This is probably only an attractive attack for a buyer anyway, at
least with the earlier commits.)
Strip all input witnesses from the depositTx message fields sent from
the buyer, until the last (DelayedPayoutTxSignatureResponse) message is
sent, where they can be bundled in as an extra field. Since the witness
data doesn't affect the final deposit tx id, the seller does not need to
know it until actually publishing the tx.
In the (fully) segwit case, this allows the buyer to prevent the seller
from publishing the deposit tx until the buyer has a valid, fully signed
delayedPayoutTx. Provide the final witness data in an extra 'depositTx'
field in DelayedPayoutTxSignatureResponse, which the seller can merge
with his depositTx witness block (for his own input signatures).
Improve validation of the buyer's delayed payout tx (both before & after
they get the final DepositTxAndDelayedPayoutTxMessage from the peer), by
finalising it independently of the seller. This is now possible since
their 2-of-2 signature is included in the DelayedPayoutSignatureRequest.
Check that the final delayedPayoutTx received from the seller matches it
byte-for-byte (which actually makes its receipt redundant now).
This also fixes an apparent security bug, where the final validation of
the delayedPayoutTx appears to skip any kind of signature check (only a
deposit tx hash check, which is still necessary).
Finally, optimistically check the deposit tx against the input of the
prepared delayedPayoutTx received from the seller, in the case that the
former is non-malleable (that is, the fully segwit case) and thus has a
stable ID given by the hash of the buyer's preparedDepositTx.
Include a new 'delayedPayoutTxSellerSignature' field with the prepared
delayed payout tx sent to the buyer, in DelayedPayoutTxSignatureRequest.
This will allow the buyer to compute the final, signed delayedPayoutTx
as early as possible and withhold their deposit tx witness from the
seller until they know they have a valid delayedPayoutTx, preventing its
premature publishing in the fully segwit case. (To be done in a later
commit - for now just save the seller's delayedPayoutTx signature.)
As part of this, run the SellerSignsDelayedPayoutTx trade task at an
earlier step (just after payout tx creation) to make its signature
available to the seller ASAP. Also rename 'delayedPayoutTxSignature' to
'delayedPayoutTxBuyerSignature' in DelayedPayoutTxSignatureResponse.
Make sure witness data is stripped from the seller's prepared deposit
tx, in addition to ScriptSig data, to prevent the buyer from being able
to publish it prematurely (before having signed the delayed payout tx).
These are failing on the tip of release/1.5.0 currently due to extra
validation added to PersistenceManager, causing the build to fail upon
merging upstream. Add missing PersistenceManager.shutDown calls to the
tearDown methods of the affected tests to fix.
We need to set addDecryptedDirectMessageListener without
delay as otherwise we could miss direct messages (detected
with localhost testing, with tor its likely slower and
would not have been triggered).
This is not really needed as we call it at each state change of the
trade but gives more redundancy in case we missed one or once
changes are applied and a dev forgets to call it.
Multiple repeated calls do have close to zero costs.
The deposit confirmed state is set after we applied the mailbox messages,
which led to a task failure due wrong phase and the message was not applied.
Further it can be that the wallet is still syncing and the deposit
confirmed state is set in any time in the future.
To fix the first problem we add a bit of delay so that the trade has
been updated when we apply the mailbox messages. A better fix would be to change
the order of the methods but that is a bit tricky to get right and I dont want to
risk that for that release.
The second problem would require a large change to trigger the mailbox
processing based on wallet state. We prefer to be more tolerant with
the expected phase instead so allow the mailbox message to be processed
also in the DEPOSIT_PUBLISHED state.
This has no risks as the payout tx would be invalid anyway if the
buyer has cheated and sent the msg in not confirmed deposit tx state (only
possible with code manipulation).
A better fix would to add a listener for the wallet and process
the mailbox msg once wallet is ready and trade state set, but I
leave that for another PR.
We relied on the shutdwon routine to be called reliably but it seems that is not the case as some bug reports show.
So we call requestPersistence at every write access of the trade object
Seems the persistence at shutdown is too unsafe and we got bug reports where data was missing.
https://github.com/bisq-network/bisq/issues/4806
Use millisec instead of sec for delay
Rename delayInSec to delay
(which can happen in rare cases) and add guards that we never create multiple instances
for a given file as well not call initialize or other API methods after shutdown was started.
It should be only needed in case we get the historical data from resources,
but as I have seen multiple times that some nodes have duplicated entries
in the live data I think its more safe to clean up always. If no entries are
removed the call is very cheap. Even with 60k entries to be pruned it takes
only about 20 ms.
As we might have same keys in multiple maps and merge those to 1 map we
cannot use an immutable map when merging the maps. Instead we copy our merged map
at the end into a immutable map.
Fix issue with immutable maps.
As we might have same keys in multiple maps and merge those to 1 map we
cannot use an immutable map when merging the maps. Instead we copy our merged map
at the end into a immutable map.
* Both axes use autoRange, which results in better
rounded values for the tick mark labels numbers.
* Filter-out data outside the factor limits.
* New constant chartDataFactor for factor limits.
* Formatted tick label currency numbers:
- Fiat: display with grouping separators
- Cryptos: remove trailing zeros in fractional parts
* Add CSS class axisy for y-axis formatting.
