RPC API: Add optional txfee property for single joins #1612

kristapsk · 2023-12-02T20:39:57Z

Resolves #1607.

kristapsk · 2023-12-02T20:41:05Z

@theborakompanioni Please test.

theborakompanioni · 2023-12-05T14:53:22Z

@theborakompanioni Please test.

I will be able to test this and report back tomorrow. Thanks @kristapsk 🙏

theborakompanioni · 2023-12-06T12:37:02Z

@kristapsk Generally works great and as expected.

Except, when an invalid value (e.g. -1) is provided, it will not reset tx_fees to the previous value.

ℹ️ Also happens when the value (+ random factor) results in a fee greater than absurd_fee_per_kb.

e.g. POST v1/wallet/Satoshi.jmdat/taker/direct-send

{"mixdepth":0,"amount_sats":0,"destination":"bcrt1q...","txfee":-1}

Returns:

<html><head><title>Processing Failed</title></head><body><b>Processing Failed</b></body></html>

(Jam parses html responses of the API and just displayes "Processing Failed")

A subsequent request to POST v1/wallet/Satoshi.jmdat/configget with

{"section":"POLICY","field":"tx_fees"}

will yield

{"configvalue": "-1"}

kristapsk · 2023-12-06T12:42:59Z

Except, when an invalid value (e.g. -1) is provided, it will not reset tx_fees to the previous value.

ℹ️ Also happens when the value (+ random factor) results in a fee greater than absurd_fee_per_kb.

This happens only with direct send or also with coinjoin?

theborakompanioni · 2023-12-06T16:04:59Z

This happens only with direct send or also with coinjoin?

On v1/wallet/Satoshi.jmdat/taker/coinjoin with

{"mixdepth":0,"amount_sats":0,"destination":"bcrt1q...","counterparties": 8,"txfee":-1}

I get no error but the "invalid" value is persisted.

In the logs I see:

2023-12-06 16:00:58,084 [DEBUG]  rpc: estimatesmartfee [-1]

without the transaction moving forward.

kristapsk · 2023-12-06T16:12:54Z

Currently sendpayment.py just ignores values <= 0. Will it be correct behaviour for RPC API to raise InvalidRequestFormat() instead?

kristapsk · 2023-12-06T17:59:33Z

Sanity checks on input should be added here, but that's only part of the problem. Direct send catches AssertionError and NotEnoughFundsException currently, but no others. Ok, here we could add just generic Exception catch and handle it there. But with coinjoin it's more difficult. It looks that in some cases on_finished_callback is not executed. @AdamISZ WDYT, how that should be handled? Go through all Taker code and make sure we always catch everything and call on_finished_callback(False, ...)?

AdamISZ · 2023-12-06T18:02:12Z

Sanity checks on input should be added here, but that's only part of the problem. Direct send catches AssertionError and NotEnoughFundsException currently, but no others. Ok, here we could add just generic Exception catch and handle it there. But with coinjoin it's more difficult. It looks that in some cases on_finished_callback is not executed. @AdamISZ WDYT, how that should be handled? Go through all Taker code and make sure we always catch everything and call on_finished_callback(False, ...)?

Will try to look at it in the next day or so.

theborakompanioni · 2023-12-06T19:56:15Z

Just as a hint: Jam will not let the user input invalid values for txfee that will be passed to these endpoints (except maybe a value that might lead to a fee greater than absurd_fee_per_kb - but that can be addressed client side).
Afaik, the setconfig endpoint does not validate inputs in general, so this would be a "separate" issue theoretically - but is not a real problem in practice. Whether the value should be validated is something you have to decide. I'd say it would be nice and something an API could do, but on the other hand, as a user of an API, you have to read its documentation anyway.

So @AdamISZ, please do not just prioritize this based on my comments. 🙏

kristapsk · 2023-12-06T20:11:20Z

Added txfee >= 0 check.

kristapsk · 2023-12-06T20:13:02Z

So @AdamISZ, please do not just prioritize this based on my comments. 🙏

The problem is that there could be a lot of random issues not related to txfee value, basically, anything that will make single join to fail with uncaught exception will restult in tx fee settings not being restored.

kristapsk · 2023-12-07T05:19:08Z

@theborakompanioni In any case, my worries above aren't directly related to this PR, that's broader issue. Could you test again this against your original issue (-1 txfee, absurd fee is not checked)?

kristapsk · 2023-12-07T05:29:05Z

Added also handling any exception for direct-send and restoring txfee there. But there's still potential problems with coinjoins.

src/jmclient/wallet_rpc.py

AdamISZ · 2023-12-14T15:46:05Z

Sanity checks on input should be added here, but that's only part of the problem. Direct send catches AssertionError and NotEnoughFundsException currently, but no others. Ok, here we could add just generic Exception catch and handle it there. But with coinjoin it's more difficult. It looks that in some cases on_finished_callback is not executed. @AdamISZ WDYT, how that should be handled? Go through all Taker code and make sure we always catch everything and call on_finished_callback(False, ...)?

OK looking at it today, I see your question.

I think it's just a matter of putting it in stop_taker.

Indeed, if we can't rely on that code being executed at the end of the single-coinjoin operation (whether it succeeds or fails), then we have other problems: there is a state-tracking variable that has to change from CJ_TAKER_RUNNING to CJ_NOT_RUNNING and if that doesn't execute, the daemon will not be able to do anything else (because it will think the Taker operation is still in process).

But if I understood what you wrote correctly, you're saying that JMWalletDaemon.taker_finished (which is the injected on_finished_callback) doesn't get called? If that happens then, according to previous paragraph, there is a separate problem, no?

AdamISZ · 2023-12-14T15:56:38Z

OK, looking at your code patch again I see you have put it just after the call to stop_taker in taker_finished. I believe it should be inside the stop_taker function as that is called twice in taker_finished (the other time is for the last transaction of a tumbler run).

kristapsk · 2023-12-29T13:04:49Z

But if I understood what you wrote correctly, you're saying that JMWalletDaemon.taker_finished (which is the injected on_finished_callback) doesn't get called? If that happens then, according to previous paragraph, there is a separate problem, no?

It looks to me so. Otherwise in any case it should log "Coinjoin did not complete successfully." Or maybe I didn't understand correctly what @theborakompanioni wrote above:

This happens only with direct send or also with coinjoin?

On v1/wallet/Satoshi.jmdat/taker/coinjoin with
{"mixdepth":0,"amount_sats":0,"destination":"bcrt1q...","counterparties": 8,"txfee":-1}
I get no error but the "invalid" value is persisted.

In the logs I see:
2023-12-06 16:00:58,084 [DEBUG]  rpc: estimatesmartfee [-1]
without the transaction moving forward.

And, yes, then it is broader problem.

theborakompanioni · 2024-01-02T15:18:47Z

It looks to me so. Otherwise in any case it should log "Coinjoin did not complete successfully." Or maybe I didn't understand correctly what @theborakompanioni wrote above [...]

This was in response to negative fee values which you apparently already addressed. Please excuse my unresponsiveness regarding this, I hope I can carve out a some more time in the coming weeks. 🙏

kristapsk · 2024-01-02T15:23:19Z

It looks to me so. Otherwise in any case it should log "Coinjoin did not complete successfully." Or maybe I didn't understand correctly what @theborakompanioni wrote above [...]

This was in response to negative fee values which you apparently already addressed. Please excuse my unresponsiveness regarding this, I hope I can carve out a some more time in the coming weeks. 🙏

And negative fee value caused exception and looks to me that this caused on_finished_callback to not execute. If it is so, the problem is that this will happen with any other exceptions too.

theborakompanioni · 2024-01-17T13:59:53Z

Latest tests on regtest led to the following:

e.g. starting a taker operation providing a negative txfee to api/v1/wallet/Satoshi.jmdat/taker/coinjoin

{"mixdepth":3,"amount_sats":0,"destination":"...","counterparties":8,"txfee":-1}

lead to an expected 400 Bad Request response:

{"message": "Invalid request format."}

However, the /session endpoints still reports an active taker operation:

{"session": true, "maker_running": false, "coinjoin_in_process": true, ...}

A subsequent request to /api/v1/wallet/Satoshi.jmdat/taker/stop will result in 500 Internal Server Error:

<html><head><title>Processing Failed</title></head><body><b>Processing Failed</b></body></html>

There is no way to resolve this situation other than restarting the daemon.

Every other test seemed to work fine!
However, it reveals that a consumer of the API definitely needs some kind of fee estimation in the future to be able to provide proper information to a user (e.g. to mitigate situations like this: joinmarket-webui/jam#613 (comment)) - but this is a different issue and can be discussed elsewhere).

kristapsk · 2024-01-20T08:44:40Z

A subsequent request to /api/v1/wallet/Satoshi.jmdat/taker/stop will result in 500 Internal Server Error:
<html><head><title>Processing Failed</title></head><body><b>Processing Failed</b></body></html>

This output is not generated by JoinMarket code. Caused by invalid JSON? Any ideas how to debug that?

theborakompanioni · 2024-01-21T14:19:01Z

A subsequent request to /api/v1/wallet/Satoshi.jmdat/taker/stop will result in 500 Internal Server Error:
<html><head><title>Processing Failed</title></head><body><b>Processing Failed</b></body></html>
This output is not generated by JoinMarket code. Caused by invalid JSON? Any ideas how to debug that?

Possibly an uncaught exception. Unfortunately, there is no output in the logs. Clients should prevent sending negative fee values in the first place, so this should not be a problem for actual users. However, just wanted to inform you about this behaviour. I hope I could give you the steps to reproduce the problem locally.

kristapsk · 2024-01-25T14:13:07Z

Clients should prevent sending negative fee values in the first place, so this should not be a problem for actual users.

The thing is that RPC backend code checks for negative fee values and this should not happen. https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/1612/files#diff-669a0b743055398b7266dcc7762ac5e2b3e4af1601237d22c689655cfc91de0eR1322

kristapsk · 2024-01-25T14:16:44Z

Rebased to run OpenAPI Diff CI action.

kristapsk · 2024-01-25T14:54:37Z

It looks OpenAPI DIff doesn't like that there is txfee property with type integer for taker requests and with type string for maker request. IMHO it's wrong that StartMakerRequest uses string there, but we can't change it without breaking compatibility, right? @theborakompanioni ?

$ grep -A 2 txfee docs/api/wallet-rpc.yaml
        txfee:
          type: integer
          example: 6
--
        - txfee
        - cjfee_a
        - cjfee_r
--
        txfee:
          type: string
          example: "0"
--
              txfee:
                type: integer
              cjfee:
--
        txfee:
          type: integer
          example: 6

theborakompanioni · 2024-01-25T16:02:59Z

Clients should prevent sending negative fee values in the first place, so this should not be a problem for actual users.

The thing is that RPC backend code checks for negative fee values and this should not happen. https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/1612/files#diff-669a0b743055398b7266dcc7762ac5e2b3e4af1601237d22c689655cfc91de0eR1322

Hmm... 🤔 Maybe I have not used the latest changes by mistake.. I will try again asap.

It looks OpenAPI DIff doesn't like that there is txfee property with type integer for taker requests and with type string for maker request. IMHO it's wrong that StartMakerRequest uses string there, but we can't change it without breaking compatibility, right? @theborakompanioni ?

Yes, it'd be good to have a grace period for some time, e.g. next version could deprecated txfee as string and introduce a new property (different name) as integer that takes precedence. Removing the txfee in the version after that.

theborakompanioni · 2024-01-26T20:28:13Z

Clients should prevent sending negative fee values in the first place, so this should not be a problem for actual users.

The thing is that RPC backend code checks for negative fee values and this should not happen. https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/1612/files#diff-669a0b743055398b7266dcc7762ac5e2b3e4af1601237d22c689655cfc91de0eR1322

I think it stems from the fact that activate_coinjoin_state is called before the txfee property validation.
Other than that, it seems to work as expected. 🙌

kristapsk · 2024-01-26T22:05:12Z

Clients should prevent sending negative fee values in the first place, so this should not be a problem for actual users.

The thing is that RPC backend code checks for negative fee values and this should not happen. https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/1612/files#diff-669a0b743055398b7266dcc7762ac5e2b3e4af1601237d22c689655cfc91de0eR1322

I think it stems from the fact that activate_coinjoin_state is called before the txfee property validation. Other than that, it seems to work as expected. 🙌

Oh, makes sense! Will move txfee validation to before that.

But also need to think how to solve OpenAPI Diff CI failure the best way.

theborakompanioni · 2024-01-26T22:17:37Z

But also need to think how to solve OpenAPI Diff CI failure the best way.

What about keeping it a string for now (for consistency) and adapting it in an API "v2" in a future major release (v0.10.0)?

kristapsk · 2024-01-26T23:15:29Z

Ok, it seems I fixed it. What it didn't like is that any type is specified for the new optional property. Should work for us, casts to int or string are made at backend code where necessary.

kristapsk · 2024-01-27T00:28:14Z

All tests pass now.

@theborakompanioni If you could re-test with ab1481d, I'm ready to merge this.

theborakompanioni · 2024-01-27T12:24:39Z

All tests pass now.

@theborakompanioni If you could re-test with ab1481d, I'm ready to merge this.

tACK! Loved to see it merged 🧡
Sorry that it took so long for me to test.

Just for the sake of completeness: If you pass a non-number string, it will answer with a 500 html response, instead of json. But this is a general inconsistency throughout the API, and definitely nothing that should be addressed here. Jam can (somewhat) handle this, by parsing the error message from the html response.

kristapsk added the RPC-API label Dec 2, 2023

kristapsk force-pushed the wallet_rpc-coinjoin-txfee branch from 6cebc43 to dcf347a Compare December 6, 2023 20:10

kristapsk force-pushed the wallet_rpc-coinjoin-txfee branch from dcf347a to e1b6fcb Compare December 7, 2023 05:27

kristapsk commented Dec 7, 2023

View reviewed changes

src/jmclient/wallet_rpc.py Show resolved Hide resolved

theborakompanioni mentioned this pull request Jan 21, 2024

feat: custom tx fee on direct and collaborative send joinmarket-webui/jam#706

Merged

1 task

kristapsk force-pushed the wallet_rpc-coinjoin-txfee branch from e1b6fcb to 421ca4b Compare January 25, 2024 14:16

kristapsk force-pushed the wallet_rpc-coinjoin-txfee branch from 421ca4b to 5d337c2 Compare January 26, 2024 22:09

kristapsk force-pushed the wallet_rpc-coinjoin-txfee branch 3 times, most recently from 9681c06 to 1061a76 Compare January 26, 2024 23:09

RPC API: Add optional txfee property for single joins

ab1481d

kristapsk force-pushed the wallet_rpc-coinjoin-txfee branch from 1061a76 to ab1481d Compare January 26, 2024 23:13

kristapsk merged commit d21f7c0 into JoinMarket-Org:master Jan 27, 2024
9 checks passed

kristapsk deleted the wallet_rpc-coinjoin-txfee branch January 27, 2024 12:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RPC API: Add optional txfee property for single joins #1612

RPC API: Add optional txfee property for single joins #1612

kristapsk commented Dec 2, 2023

kristapsk commented Dec 2, 2023

theborakompanioni commented Dec 5, 2023

theborakompanioni commented Dec 6, 2023

kristapsk commented Dec 6, 2023

theborakompanioni commented Dec 6, 2023

kristapsk commented Dec 6, 2023

kristapsk commented Dec 6, 2023

AdamISZ commented Dec 6, 2023

theborakompanioni commented Dec 6, 2023 •

edited

Loading

kristapsk commented Dec 6, 2023

kristapsk commented Dec 6, 2023 •

edited

Loading

kristapsk commented Dec 7, 2023

kristapsk commented Dec 7, 2023

AdamISZ commented Dec 14, 2023 •

edited

Loading

AdamISZ commented Dec 14, 2023

kristapsk commented Dec 29, 2023

theborakompanioni commented Jan 2, 2024

kristapsk commented Jan 2, 2024

theborakompanioni commented Jan 17, 2024

kristapsk commented Jan 20, 2024

theborakompanioni commented Jan 21, 2024 •

edited

Loading

kristapsk commented Jan 25, 2024

kristapsk commented Jan 25, 2024

kristapsk commented Jan 25, 2024

theborakompanioni commented Jan 25, 2024

theborakompanioni commented Jan 26, 2024

kristapsk commented Jan 26, 2024

theborakompanioni commented Jan 26, 2024 •

edited

Loading

kristapsk commented Jan 26, 2024

kristapsk commented Jan 27, 2024

theborakompanioni commented Jan 27, 2024

RPC API: Add optional txfee property for single joins #1612

RPC API: Add optional txfee property for single joins #1612

Conversation

kristapsk commented Dec 2, 2023

kristapsk commented Dec 2, 2023

theborakompanioni commented Dec 5, 2023

theborakompanioni commented Dec 6, 2023

kristapsk commented Dec 6, 2023

theborakompanioni commented Dec 6, 2023

kristapsk commented Dec 6, 2023

kristapsk commented Dec 6, 2023

AdamISZ commented Dec 6, 2023

theborakompanioni commented Dec 6, 2023 • edited Loading

kristapsk commented Dec 6, 2023

kristapsk commented Dec 6, 2023 • edited Loading

kristapsk commented Dec 7, 2023

kristapsk commented Dec 7, 2023

AdamISZ commented Dec 14, 2023 • edited Loading

AdamISZ commented Dec 14, 2023

kristapsk commented Dec 29, 2023

theborakompanioni commented Jan 2, 2024

kristapsk commented Jan 2, 2024

theborakompanioni commented Jan 17, 2024

kristapsk commented Jan 20, 2024

theborakompanioni commented Jan 21, 2024 • edited Loading

kristapsk commented Jan 25, 2024

kristapsk commented Jan 25, 2024

kristapsk commented Jan 25, 2024

theborakompanioni commented Jan 25, 2024

theborakompanioni commented Jan 26, 2024

kristapsk commented Jan 26, 2024

theborakompanioni commented Jan 26, 2024 • edited Loading

kristapsk commented Jan 26, 2024

kristapsk commented Jan 27, 2024

theborakompanioni commented Jan 27, 2024

theborakompanioni commented Dec 6, 2023 •

edited

Loading

kristapsk commented Dec 6, 2023 •

edited

Loading

AdamISZ commented Dec 14, 2023 •

edited

Loading

theborakompanioni commented Jan 21, 2024 •

edited

Loading

theborakompanioni commented Jan 26, 2024 •

edited

Loading