fix: precondition for collaborative transactions #485

theborakompanioni · 2022-08-25T11:12:12Z

Closes #438.

Adds CoinjoinRequirements as replacement for CoinjoinPreconditions.

Changes:

minimum confirmations limit (>=5 confs) must be fulfilled for all given utxos (instead of single utxo)
- for single collaborative tx: min conf limit for utxos in source jar
- for scheduler: min conf limit for utxos in all jars
at least one retry must be available
- for single collaborative tx: at least one retry left in source jar
- for scheduler: at least one retry left in all (funded) jars
do not disable the "Start Scheduler" button on the Sweep page when displaying "precondition warnings"
- with latest upstream changes, the scheduler will wait and retry e.g. if utxos with less than 5 confs are present
- also, if no retries are left, the scheduler might rectify this situation (sometimes, not always)
- Currently, the "Send" button on the send page is still disabled, as there are no retry mechanisms in place. The source jar is known so there should be no false positives.

Regtest:

regtest: set taker_utxo_retries and maker_timeout_sec config vars for improved testability
- default of taker_utxo_retries is 3 - reduce to 1 to easily "simulate" a jar without retries left
- default of maker_timeout_sec is 30 - making it 10 will increase chances of a maker timeout and additionally invokes the "Stall monitor" more often

Tests:

3 utxos with >5 confs in Jar A: Success.
3 utxos with >5 confs distributed in Jar A, Jar B and Jar C: Success.
2 utxos with >5 confs and 1 utxo with 1conf in Jar A, 1 utxo with 1conf in Jar B: Fail -> fixed
3 utxos with >5 confs in Jar A, 1 utxo with 0conf in Jar B: Fail -> fixed
2 utxos with >5 confs in Jar A, 1 utxo with >5confs in Jar B, 1 utxo with 4 confs in Jar E: Fail: Fail -> fixed

2 utxos with >5 confs in Jar A, 2 utxo (one without retries left) in Jar B: Success!

Additional tests after JoinMarket-Org/joinmarket-clientserver#1347 has been merged:

Scheduler continuing (with orderbook being initially empty) after an order was broadcasted after starting
2 utxos with >5 confs in Jar A, 1 utxo without retries left in Jar B: Fail -> fixed

(slightly outdated screenshot - the info is displayed as warning (yellow) instead of an error (red))

How to test

Minimum value requirement

Not implemented - reasons stated below

Minimum confirmation requirement (>5 confs)

Try using the scheduler or send page with a jar having less then 5 confirmations.

No retries left

To just see the "retries left" error message add the following snipped temporarily to the Jam.jsx component.

  walletInfo?.data.utxos.utxos.forEach((it) => it.tries_remaining = 0)

To really emulate and test having a utxo without retries left, you need to manually abort a taker operation after a commitment has been sourced. To do this you can send a collaborative transaction from one jar to another, while observing the logs (npm run regtest:logs:jmwalletd) and immediately lock the wallet once you see the log message INFO:Commitment sourced OK. You can test various scenarios, e.g. having only one utxo without retries left (tx will fail - message should appear) or having 1 utxo without retries left + 1 utxo with retries left (tx can succeed - message should NOT appear).

The message should only appear if all utxos in a jar have no retries left.

Not addressed:

Utxos that were not at least 20% of the size of the coinjoin amount
- Could not reproduce this behaviour consistently (e.g. single collab tx with input 1rBTC and 2500rsats was successful)
- Difficult to reproduce as utxos are prefiltered by JM and it is not trivial to rebuild this behaviour client-side
- Furthermore, would need a way to get the actual tx amount during the scheduler reliably

e.g. sending 30.1rBTC (3010000000 sats) with two utxos (30rBTC and 0.12345678rBTC) was successful

2022-09-10 17:46:40,443 [DEBUG]  schedule item was: [4, 3010000000, 1, 'bcrt1q9dwpt0x33k8a7nfdrpej9lfsmk7q4xquxkgas7', 0, 16, 0]
[...]
2022-09-10 17:46:44,310 [INFO]  Removed utxos=
d11fc2f089d672b4a7d2784062b5b343315dcdd43894fa508d8d5a1356631d90:1 - path: m/84'/1'/4'/0/12, address: bcrt1qtn5jlr74lfg4cf5pcvgchytt4dkn3yq3205ntp, value: 12345678
4af2681b928893045eba143b6c08d83248527d0d52e9e7ed3b5f186c36e630da:0 - path: m/84'/1'/4'/0/11, address: bcrt1q6nwjgnf6qg4yejytxnyncf9lm0glxzttwusv2d, value: 3000000000

sweeping three utxos (~76rBTC, ~21rBTC and 0.02341166rBTC) was also successful

2022-09-10 17:58:24,873 [DEBUG]  schedule item was: [0, 0, 1, 'bcrt1qfqe4kkm940vx8fm0jcycu44tfktmtw38ga7ld4', 0, 16, 0]
[...]
2022-09-10 17:58:29,281 [INFO]  Removed utxos=
b3b57af79255753a7798133738942e511a2fda09a26822317ad98cea1462b749:0 - path: m/84'/1'/0'/0/11, address: bcrt1q79ne2hnuxv47pz078rfnq39n26l0u65utr5433, value: 2341166
c58b15e3f09c4f2842449673ea4e3b5284a7fbfb6ea5c2486c57b1a98975e192:0 - path: m/84'/1'/0'/0/9, address: bcrt1q37jxrr28tf8kerx4hs8d9ev6rad2mxglylcfd9, value: 7616114702
9f1fbb728ba994083e37bbf20acd4c6789b00d6a77a4f0a0bd52738163181948:0 - path: m/84'/1'/0'/0/10, address: bcrt1qyce0vz6zfkkds7t875u0dxwehwf3t3gu8a3gq9, value: 2124668118

However, the following (with a sweep) did not succeed:

2022-09-10 18:18:52,572 [DEBUG]  cj amount = 2246731806
2022-09-10 18:18:52,594 [INFO]  ABORT:Failed to source a commitment; this debugging information may help:

1: Utxos that passed age and size limits, but have been used too many times (see taker_utxo_retries in the config):
d11fc2f089d672b4a7d2784062b5b343315dcdd43894fa508d8d5a1356631d90:0
2: Utxos that have less than 5 confirmations:
None
3: Utxos that were not at least 20% of the size of the coinjoin amount 2246731806
165c596ab9711b46536364182725167a1d912efee0630c9d8fe095552bcf098b:0
***
Utxos that appeared in item 1 cannot be used again.
Utxos only in item 2 can be used by waiting for more confirmations, (set by the value of taker_utxo_age).
Utxos only in item 3 are not big enough for this coinjoin transaction, set by the value of taker_utxo_amtpercent.
If you cannot source a utxo from your spending mixdepth according to these rules, use the tool add-utxo.py to source a utxo from another mixdepth or a utxo external to your joinmarket wallet. Read the help with 'python add-utxo.py --help'

***
For reference, here are the utxos in your wallet:

mixdepth 4:
    be817dee9eafb9199609109b7f36e608aa6164f70cd57446a88b26d4b043b514:2 - path: m/84'/1'/4'/0/13, address: bcrt1qfqe4kkm940vx8fm0jcycu44tfktmtw38ga7ld4, value: 9743118854
mixdepth 3:
    2e8316092b08eccf353e2754199084610b25173dff98a2c1b354b57d058b3127:2 - path: m/84'/1'/3'/0/5, address: bcrt1q9dwpt0x33k8a7nfdrpej9lfsmk7q4xquxkgas7, value: 3010000000
mixdepth 1:
    d11fc2f089d672b4a7d2784062b5b343315dcdd43894fa508d8d5a1356631d90:0 - path: m/84'/1'/1'/1/11, address: bcrt1q438f9hg99p8edhafdmafremrpy69sfqh3fxk47, value: 2073019140
    165c596ab9711b46536364182725167a1d912efee0630c9d8fe095552bcf098b:0 - path: m/84'/1'/1'/0/3, address: bcrt1qsvj777cuq5de65grj4x6k3uyxrrnwxursx98ky, value: 173717118

BUT, if the same jar is sourced with a new utxo and swept, it will succeed:

2022-09-12 18:27:47,614 [DEBUG]  cj amount = 11989848869
[...]
2022-09-10 18:27:56,922 [DEBUG]  schedule item was: [1, 0, 1, 'bcrt1qd60sjmf25mj3ze8kc9sa5lcs7tp692hr2g0j5s', 0, 16, 0]
[...]
2022-09-10 18:27:58,048 [INFO]  Removed utxos=
3cbee223000ee40195706acb080e427988cb76c3befc207c61adfa6157a78688:0 - path: m/84'/1'/1'/0/4, address: bcrt1qjzuaucz8zd3d30xxauygxrxv03kajkaaalhjcw, value: 9743117744
165c596ab9711b46536364182725167a1d912efee0630c9d8fe095552bcf098b:0 - path: m/84'/1'/1'/0/3, address: bcrt1qsvj777cuq5de65grj4x6k3uyxrrnwxursx98ky, value: 173717118
d11fc2f089d672b4a7d2784062b5b343315dcdd43894fa508d8d5a1356631d90:0 - path: m/84'/1'/1'/1/11, address: bcrt1q438f9hg99p8edhafdmafremrpy69sfqh3fxk47, value: 2073019140
```

theborakompanioni · 2022-08-25T16:51:00Z

Not addressed currently is handling of "tries exceeded":

2 utxos with >5 confs in Jar A, 1 utxo without retries left in Jar B: Fail -> not addressed atm
2 utxos with >5 confs in Jar A, 1 utxo without retries left in Jar B, 1 utxo with >5confs in Jar C: similar as above -> not addressed atm
2 utxos with >5 confs in Jar A, 2 utxo (one without retries left) in Jar B: Success!

Setup

Jar A: some utxos with >5 confs
Jar B: 1 utxo with >5 confs but without retries left

Fails with:

2022-08-24 10:08:00,338 [INFO]  ABORT:Failed to source a commitment; this debugging information may help:

1: Utxos that passed age and size limits, but have been used too many times (see taker_utxo_retries in the config):
1dee3490762fd03abc7058cf8d333a7cd659cea2088ccdbf3ec962b7d48f0f16:0
2: Utxos that have less than 5 confirmations:
None
3: Utxos that were not at least 20% of the size of the coinjoin amount 123451658
None
***
Utxos that appeared in item 1 cannot be used again.
Utxos only in item 2 can be used by waiting for more confirmations, (set by the value of taker_utxo_age).
Utxos only in item 3 are not big enough for this coinjoin transaction, set by the value of taker_utxo_amtpercent.
If you cannot source a utxo from your spending mixdepth according to these rules, use the tool add-utxo.py to source a utxo from another mixdepth or a utxo external to your joinmarket wallet. Read the help with 'python add-utxo.py --help'

***
For reference, here are the utxos in your wallet:

mixdepth 1:
    1dee3490762fd03abc7058cf8d333a7cd659cea2088ccdbf3ec962b7d48f0f16:0 - path: m/84'/1'/1'/0/3, address: bcrt1qpdzjev9s6eg3knrh8vjx55adl0fvrehh08spsc, value: 123455679
mixdepth 0:
    268d61ecd16fe05ff2baf47c3e829a3a30b927019b1a043d429c86defaf2b717:0 - path: m/84'/1'/0'/0/5, address: bcrt1qvayhk7q52f68ux2s3chj7x3k732vw7vluysrjd, value: 7053069886
    c41535c3a7dc0e8d6849da11c974b55645cd9756d527d60b6ff6beb618f0ae2e:3 - path: m/84'/1'/0'/1/2, address: bcrt1qactg6y6gk6d3w0jntn9qsmpf6wjk5rtqepa5m0, value: 4612135841
    2e58ef0ada7351212c55c5925141294fb19f5a44ca594579e4b8581738c07caa:1 - path: m/84'/1'/0'/1/3, address: bcrt1qewah59trg5w8rpycm4k7txdggvzj5sfdltjrd0, value: 3087813474
    5ff7de389f8df9d2b45d238cbfdc67c9ccf0a72adde6530bc853415735d8c554:1 - path: m/84'/1'/0'/1/4, address: bcrt1qfxwzzvn69vgl90tjnl0h69lqnersdjqxzfljxl, value: 123448747

New scheduler algorithm will first try to sweep from Jar B, which fails because no retries are left:

cat TUMBLE.schedule 
1,0,1,INTERNAL,0.0,16,0
0,0,1,INTERNAL,0.01,16,0
1,0.4003229342268936,1,INTERNAL,0.16,16,0
1,0,1,bcrt1qvr3yw3lq4ajc8ltw5x380k90xtxq39c5j8ltup,0.08,16,0
2,0.6721412773748464,1,INTERNAL,0.11,16,0
2,0,1,bcrt1qmhxlgxzfwjzjjz3k2agww8hteayygvfh5nd3dl,0.03,16,0
3,0,1,bcrt1q0fx7v9nyrlnxndzd36eld62h2s8fmlam425z0r,0.03,16,0

theborakompanioni · 2022-08-25T17:00:38Z

Undrafting this now, as the "no tries left" issue can be tackled in a follow-up PR. What do you think about continuously checking the current schedule entry and check if the current "sending Jar" is checked to have enough retries left?
A warning could be shown that indicates when the scheduler might got stuck..
Of course, this is not a proper solution, but until we get back error states or some indication of a "stuck" scheduler, this might be a way to at least let the user know.

dergigi · 2022-09-09T09:24:32Z

Undrafting this now, as the "no tries left" issue can be tackled in a follow-up PR. What do you think about continuously checking the current schedule entry and check if the current "sending Jar" is checked to have enough retries left? A warning could be shown that indicates when the scheduler might got stuck...

I think showing a warning is fine for now, it's better than nothing. Let's try to tackle this properly at a later point.

src/components/Send.jsx

could not consistently reproduce that this requirement is really needed

…rs for improved testability

theborakompanioni · 2022-09-13T10:12:41Z

Updated the PR description to match new code adaptions.
Other comment are outdated. Please feel free to be harsh on feedback, as this is meant as a "suggestion".
Highly appreciate any and all inputs.

dergigi

Looks good to me in general, I did my best to test this is thoroughly as I could.

I didn't find any bugs, but I noticed that the behavior of send/sweep is now inconsistent. (By "sweep" I mean the scheduler.)

We disable the send button for a “normal” send if preconditions are met, but we don’t disable the scheduler button if the preconditions aren't met.

I see two options:

Enable the regular send button as well & reword the warnings so that stuff makes sense, which it currently doesn’t (we say that “the scheduler requires” - but you can still start it, so it doesn’t really “require” it does it) - maybe even make the button red or orange, as we did when creating an FB with a non-cjout UTXO
Disable the scheduler button if preconditions are not met

I'm leaning towards (1). This change can be addressed in another PR, of course.

dergigi · 2022-09-14T11:13:57Z

src/components/CoinjoinPreconditionViolationAlert.tsx

+            <br />
+            <br />
+            <Trans i18nKey={`${i18nPrefix}hint_missing_retries_detail`} count={utxosViolatingRetriesLeft.length}>
+              Following utxos have been used unsuccessfully too many times:


Suggested change

Following utxos have been used unsuccessfully too many times:

The following UTXOs have been used unsuccessfully too many times:

src/i18n/locales/en/translation.json

Co-authored-by: Gigi <[email protected]>

dergigi

Looks good to me, tACK ✅

I'd lean towards merging this as-is and do some final adjustments in a separate PR.

theborakompanioni · 2022-09-15T16:51:30Z

Looks good to me, tACK white_check_mark

I'd lean towards merging this as-is and do some final adjustments in a separate PR.

Okay. Thanks!
With #497 ready, I think your suggestion to enable the button on the Send page and rephrasing the warning is a good compromise. Then, if a user still attempts to do a tx with too little confirmations, it can be aborted.

theborakompanioni added the bug Something isn't working label Aug 25, 2022

theborakompanioni self-assigned this Aug 25, 2022

theborakompanioni force-pushed the preconditions branch from 5fb70da to 7eb7f4d Compare August 25, 2022 14:50

fix: min confs of preconditions for all utxos instead of single

7eb7f4d

theborakompanioni changed the title ~~fix: preconditions for collaborative transactions~~ fix: min confirmation precondition for collaborative transactions Aug 25, 2022

theborakompanioni marked this pull request as ready for review August 25, 2022 17:00

theborakompanioni requested a review from dergigi August 25, 2022 17:00

theborakompanioni added 2 commits August 26, 2022 11:43

fix: i18n phrases of collaborative transaction preconditions

85bdb34

fix: link to markdown docs instead of archived wiki page

4b478ff

dergigi reviewed Sep 9, 2022

View reviewed changes

src/components/Send.jsx Outdated Show resolved Hide resolved

Merge branch 'master' into preconditions

4fb1152

theborakompanioni marked this pull request as draft September 12, 2022 10:12

theborakompanioni added 3 commits September 12, 2022 14:19

Merge branch 'master' into preconditions

ea9b372

wip: add CoinjoinRequirements

175bb4c

wip: remove min utxo size check

5745813

could not consistently reproduce that this requirement is really needed

theborakompanioni force-pushed the preconditions branch from ec5c3e5 to 83f369c Compare September 12, 2022 12:25

wip: use new scheduler preconditions in Jam component

83f369c

theborakompanioni changed the title ~~fix: min confirmation precondition for collaborative transactions~~ fix: precondition for collaborative transactions Sep 12, 2022

theborakompanioni added 4 commits September 12, 2022 16:40

wip: externalize CoinjoinPreconditionViolationAlert component

08bea1d

fix: only report utxos without retries of no retry is left within jar

cc7b5d8

dev: readd optional min value precondition check

3f83aa0

dev: use CoinjoinPreconditionViolationAlert on Send page

999bf39

theborakompanioni requested a review from dergigi September 12, 2022 18:39

dev: remove min utxo size check (again)

b82a912

theborakompanioni force-pushed the preconditions branch from 7f6659a to fef437e Compare September 12, 2022 19:03

feat(regtest): set taker_utxo_retries and maker_timeout_sec config va…

fef437e

…rs for improved testability

theborakompanioni marked this pull request as ready for review September 12, 2022 19:15

theborakompanioni added 5 commits September 12, 2022 21:43

test(preconditions): add test for CoinjoinRequirements

8a24605

chore: remove outdated component CoinjoinPreconditions

e832cc7

test: add test for multiple jar check in CoinjoinRequirements

d4ccab6

ui: display retries info as warning intead of error

0833389

fix: enable starting scheduler even if preconditions are not fulfilled

c9fb002

dergigi reviewed Sep 14, 2022

View reviewed changes

theborakompanioni and others added 3 commits September 15, 2022 11:27

review: fix wording

63de541

Co-authored-by: Gigi <[email protected]>

Merge branch 'master' into preconditions

08989ac

Merge branch 'master' into preconditions

0c5f4ad

dergigi approved these changes Sep 15, 2022

View reviewed changes

theborakompanioni merged commit db29235 into master Sep 15, 2022

theborakompanioni deleted the preconditions branch September 15, 2022 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: precondition for collaborative transactions #485

fix: precondition for collaborative transactions #485

theborakompanioni commented Aug 25, 2022 •

edited

Loading

theborakompanioni commented Aug 25, 2022 •

edited

Loading

theborakompanioni commented Aug 25, 2022

dergigi commented Sep 9, 2022 •

edited

Loading

theborakompanioni commented Sep 13, 2022

dergigi left a comment

dergigi Sep 14, 2022

dergigi left a comment

theborakompanioni commented Sep 15, 2022

	Following utxos have been used unsuccessfully too many times:
	The following UTXOs have been used unsuccessfully too many times:

fix: precondition for collaborative transactions #485

fix: precondition for collaborative transactions #485

Conversation

theborakompanioni commented Aug 25, 2022 • edited Loading

How to test

Minimum value requirement

Minimum confirmation requirement (>5 confs)

No retries left

theborakompanioni commented Aug 25, 2022 • edited Loading

Setup

theborakompanioni commented Aug 25, 2022

dergigi commented Sep 9, 2022 • edited Loading

theborakompanioni commented Sep 13, 2022

dergigi left a comment

Choose a reason for hiding this comment

dergigi Sep 14, 2022

Choose a reason for hiding this comment

dergigi left a comment

Choose a reason for hiding this comment

theborakompanioni commented Sep 15, 2022

theborakompanioni commented Aug 25, 2022 •

edited

Loading

theborakompanioni commented Aug 25, 2022 •

edited

Loading

dergigi commented Sep 9, 2022 •

edited

Loading