Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement minimum tx feerate setting for makers #1602

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kristapsk
Copy link
Member

Addressing #721. Implements only absolute feerate setting (relative is not so simple, block confirmation targets will differ between different nodes, so that means we should re-announce offers after each mined block). Upgraded takers will take this into account and not choose orders below feerate they plan to pay. Non-upgraded takers will lose coinjoin quality by upgraded makers refusing to sign if feerate is below one they desire.

It's not yet finished, some tests are failing, no manual testing have been done.

@kristapsk kristapsk added protocol related to the communication between maker / taker High priority labels Nov 23, 2023
@kristapsk kristapsk marked this pull request as draft November 23, 2023 15:56
@@ -514,6 +514,11 @@ def jm_single() -> AttributeDict:
# [fraction, 0-1] / variance around all offer sizes. Ex: 500k minsize, 0.1 var = 450k-550k
size_factor = 0.1

#
minimum_tx_fee_rate = 1000
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you share some thinking behind this number? (I mean, apart from the obvious :) ).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing apart from the obvious - it's default minimum tx relay fee.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. This might be related to point 3/ in my list of suggestions here.

You want this var to be continuously updated, at least as transactions are confirmed, but much more, also when new blocks come in, probably. I agree with you that we want that kind of frequency of update.

I didn't think about this in detail earlier today but ... YGs advertise their offers when they start, and also advertise updates in the pit on transaction confirmation or "arrival" (sometimes) but when the taker requests !orderbook they're going to privmsg their offer at that point. The problem is that that announcement-in-private (the response to the Taker's !orderbook) is handled entirely within the daemon, as per the comment to src.jmdaemon.daemon_protocol.JMDaemonServerProtocol.on_orderbook_requested, this is dealt with by the daemon assuming the offerlist is up to date.

If we wanted to make that a dynamic update, we'll have to add more code to inject updates to the offerlist, e.g. at a regular interval, or perhaps even, every time an orderbook request is received.

There's also a question of whether we want the starting value (default?) to be 1000, or maybe have a starting value be undefined, forcing it to be calculated or something like that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean that makers should never sign transactions that are below local mempoolminfee and thus will not be seen in local mempool, which means maker either will see it only after longer time when it is confirmed or will make full-RBF replacement without even knowing it?

If we cannot avoid dynamic offer updates after each block, then: 1) no point to not implement also block confimation target for this setting too, I avoided it to get MVP ready faster, 2) it is related to #1042, same problems holding it back needs to be solved.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we cannot avoid dynamic offer updates after each block

I think we can, but, it might be the case that we need some extra code for client-daemon communication, whether we choose to update immediately when the taker sends us the !orderbook request, or, say, once per block. In either case, it is more dynamic than what we do now: we only update offer details when there is a trigger for on_tx_unconfirmed or on_tx_confirmed.

This "extra code" is an extra communication type specified in jmbase.commands.py and then extra functions to send/receive message in jmdaemon.daemon_protocol.py and jmclient.client_protocol.py. If we don't do that, then as mentioned above, the daemon just reads the current offer from an internally stored offerlist variable, and doesn't know that anything changed in jmclient.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean that makers should never sign transactions that are below local mempoolminfee and thus will not be seen in local mempool, which means maker either will see it only after longer time when it is confirmed or will make full-RBF replacement without even knowing it?

I didn't specifically mean that, but sure, that is a scenario where it could cause problems. I just don't understand why it would make sense to have the exact threshold of failure (1sat/vb) to propagate, as the default.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, another reason is that values below 1000 for tx feerates are treated as block confirmation targets not sats per kvB by estimate_fee_per_kb().

@AdamISZ
Copy link
Member

AdamISZ commented Nov 24, 2023

So it seems like the only issue we have to get fixed on is as per your original description:

Implements only absolute feerate setting (relative is not so simple, block confirmation targets will differ between different nodes, so that means we should re-announce offers after each mined block).

Like I'm sure you are, I am concerned that this static approach will affect user experience too much. It would be very easy to imagine a lot of makers starting or restarting their bots with min_tx_fee=30000 or something like that during extremely high fees - they don't want their utxos stuck for days - and then when fees drop drastically to the sub-10k sats/vkb region, all the takers who sensibly didn't try to use JM with super-high fees, come back, try to do a coinjoin, and most of the makers just reject their attempts.

There's a nuance, in that scenario, which should be noted:

Case 1: the taker with the new code: since the flow is !orderbook then **[rel/absoffer] via privmsg, the taker will not !fill offers that have too high a min tx fee, so they won't use up commitments. They'll just find very few (maybe not enough) makers to join with.

Case 2: the taker with old code: they ignore the fee information, and they are very likely, in this scenario, to use up commitments and not get a join, if the large majority of makers are demanding fees > 30k sat/vkb and the taker obviously isn't abiding by that rule.

In short, I'd tend to think this PR should already try to have dynamic updates and not a static value, because of the conflict between: makers are very passive usually, not updating their bot for long periods, and, fees are very volatile.

@kristapsk
Copy link
Member Author

In short, I'd tend to think this PR should already try to have dynamic updates and not a static value, because of the conflict between: makers are very passive usually, not updating their bot for long periods, and, fees are very volatile.

If we go that route, I think we should first get back to #1042. It is simpler PR where likely needs to be figured out how we do offer updates after each block (there it is privacy issue). #1042 (comment)

@kristapsk
Copy link
Member Author

Should jmdaemon.protocol.JM_VERSION be changed with this?

@AdamISZ
Copy link
Member

AdamISZ commented Nov 25, 2023

In short, I'd tend to think this PR should already try to have dynamic updates and not a static value, because of the conflict between: makers are very passive usually, not updating their bot for long periods, and, fees are very volatile.

If we go that route, I think we should first get back to #1042. It is simpler PR where likely needs to be figured out how we do offer updates after each block (there it is privacy issue). #1042 (comment)

Hmm, it's not clear to me, but I see what you're saying.

Here, if we take the option of dynamically updating the min_tx_fee var specifically when a taker has sent !orderbook and then we are in real time monitoring our sw0reloffer response to include the update (and I prefer this version, as noted above), then the code doesn't hugely overlap.

If instead, we code it to update the min_tx_fee of the offer after every block, and publish it in pubmsg, then, that does overlap indeed.

I'm not yet sure in detail whether the former will be a bit harder to code (maybe?), but they have significant similarities. One additional reason I prefer the former is because it's less likely to cause big floods of messages in pubmsg on the channels.

@AdamISZ
Copy link
Member

AdamISZ commented Nov 26, 2023

Should jmdaemon.protocol.JM_VERSION be changed with this?

Good question. I think technically yes but ... maybe no?

Fidelity bonds had the nice property that it really felt 'fully' backwards-compatible; takers on old code saw essentially zero change in their functional experience. Old makers did have a reduction in quality in the sense that they would get joins much less often, but we are much less concerned about the maker experience (it's their job to offer a service), they should upgrade if they want to compete. At least, no bots could be broken.

Here, it's somewhat similar, and again no bots are "broken", except it's the quality of experience of the taker that's more effected, and that's more serious.

However we might have to just ignore this, I fear that updating JM_VERSION, essentially forcing everyone to upgrade or "split the pit" so to speak, is probably unnecessarily violent for this case (we would also need to update directory nodes jm_min_version and max_version (sorry forgot actual names, but it's in the handshake)). Debatable.

@kristapsk
Copy link
Member Author

Should jmdaemon.protocol.JM_VERSION be changed with this?

Good question. I think technically yes but ... maybe no?

Yeah, that was the question - should this be changed only with hard forks or also soft forks of the protocol. Probably it's easier to not, although technically it is a new protocol version.

kristapsk added a commit that referenced this pull request Nov 26, 2023
…sages

bfc618a Fix OrderbookWatch.on_order_seen() exception debug messages (Kristaps Kaupe)

Pull request description:

  You cannot concatenate `str` with `int`. Found while working on #1602.

ACKs for top commit:
  AdamISZ:
    utACK bfc618a

Tree-SHA512: 34e2181bd91c856348fee88233f76d25a51d1626d1ad3523e861caa3ba84c248371de874841038381839efa4816d58d2d0933628f79bcf7d64295483c7959dfc
@AdamISZ
Copy link
Member

AdamISZ commented Nov 26, 2023

So my best suggestion is:

1/ don't update JM_VERSION
2/ need an extra patch to this PR to send regular updates (possibly, with the "heartbeat" of the wallet_service transaction monitor, or otherwise a similar polling loop) that sends a message to the daemon to update the min_tx_fee in the offer

I'm focusing on this as a concrete suggestion because this PR is high priority.

Also, I can try to do 2/ myself, if you like.

@kristapsk
Copy link
Member Author

I am first focusing on fixing current test failures (that KeyboardInterrupt is PITA).

Feel free to try (2) independently.

Also, some new test cases for this functionality needs to be added.

@AdamISZ
Copy link
Member

AdamISZ commented Nov 26, 2023

I am first focusing on fixing current test failures (that KeyboardInterrupt is PITA).

Feel free to try (2) independently.

Also, some new test cases for this functionality needs to be added.

A thought that immediately occurs to me when I try to implement 2/ :

This PR allows the maker to set a feerate of the absolute type (not the estimate**fee type). But if we want to update it (whether every 5 seconds or every block or whatever), it seems that's like a conflicted idea - we could only update it using estimate**fee calls, right? In that case why didn't the user start by using that "relative" version?

It almost seems like we should do the opposite of this PR: in the maker's offer, only allow a minimum_tx_fee_rate using a number-of-blocks-target (estimate**fee) (like 3). This is much easier since it doesn't need to be updated, but on the other hand, as you noted in the first message in this thread, that is obviously problematic, since the taker's Core node may be using estimates that don't match. Maybe it's OK if the taker is choosing 2 and the maker is choosing 5, but, that's not exactly great.

Alternatively, we just accept that it can't be updated? i.e. a maker says "as long as this instance of the bot is running, it will only accept feerates above 20k sats/kvb". But that seems kinda dumb, somehow.

So at this point I'm not even sure what we want to do?

@AdamISZ
Copy link
Member

AdamISZ commented Nov 26, 2023

Maybe we could have the maker's initial value (say, 20k s/kvb) and multiply it proportionately by the changes in the output of estimatesmartfee? Like, if the estimate at the start is 10k, then after 1 minute it becomes 15k, we change the maker's minimum_tx_feerate from 20k to 30k?

That just seems too confusing/complicated/janky.

@kristapsk
Copy link
Member Author

Test causing KeyboardInterrupt is TrialTestJMDaemonProto.

@AdamISZ
Copy link
Member

AdamISZ commented Nov 28, 2023

I'm not sure if it's just not showing up clearly in the CI test because of the absence of the -s option, but when I run:

pytest --btcpwd=123456abcdef --nirc=2 --btcconf=/home/waxwing/bitcoin.conf -p no:warnings test/jmdaemon/test_daemon_protocol.py -s

then I get the error on the terminal:

File "/home/waxwing/joinmarket-clientserver/test/jmdaemon/test_daemon_protocol.py", line 231, in on_JM_INIT
	    self.mcc.on_order_seen_trigger(mcs[0], c, "a", "b", "c", "d", "e", "f")
	builtins.TypeError: on_order_seen_trigger() missing 1 required positional argument: 'minimum_tx_fee_rate'

which should be pretty easy to fix.

@AdamISZ
Copy link
Member

AdamISZ commented Nov 28, 2023

Indeed the following patch fixed those errors in test_daemon_protocol.py for me:

diff --git a/test/jmdaemon/test_daemon_protocol.py b/test/jmdaemon/test_daemon_protocol.py
index 925b624..7e500de 100644
--- a/test/jmdaemon/test_daemon_protocol.py
+++ b/test/jmdaemon/test_daemon_protocol.py
@@ -213,7 +213,7 @@ class JMDaemonTestServerProtocol(JMDaemonServerProtocol):
             #counterparty, oid, ordertype, minsize, maxsize,txfee, cjfee):
             self.on_order_seen(o["counterparty"], o["oid"], o["ordertype"],
                                  o["minsize"], o["maxsize"],
-                                 o["txfee"], o["cjfee"])
+                                 o["txfee"], o["cjfee"], o['minimum_tx_fee_rate'])
         return super().on_JM_REQUEST_OFFERS()
 
     @JMInit.responder
@@ -228,7 +228,7 @@ class JMDaemonTestServerProtocol(JMDaemonServerProtocol):
         #note it must happen before callign set_msgchan for OrderbookWatch
         self.mcc.on_order_seen = None
         for c in [o['counterparty'] for o in t_orderbook]:
-            self.mcc.on_order_seen_trigger(mcs[0], c, "a", "b", "c", "d", "e", "f")
+            self.mcc.on_order_seen_trigger(mcs[0], c, "a", "b", "c", "d", "e", "f", "g")

@kristapsk
Copy link
Member Author

But if we want to update it (whether every 5 seconds or every block or whatever), it seems that's like a conflicted idea - we could only update it using estimate**fee calls, right? In that case why didn't the user start by using that "relative" version?

Different users may want different things. Some might be ok with some fixed lower limit, never update it (or update rarely by manually changing configuration). You could be ok with tx being "stuck" for week or two, just charge bigger cjfees for that. And for these there is no need to update at all, especially if they have big enough local mempool. Others might want to always be more or less sure (but you can never be totally sure, of course) for, let's say, confirmation in 24 hours, then dynamic fees are needed and automatically updated, but that was what I wanted to avoid with MVP.

Alternatively, we just accept that it can't be updated? i.e. a maker says "as long as this instance of the bot is running, it will only accept feerates above 20k sats/kvb". But that seems kinda dumb, somehow.

As I said, not the best, but it's already something and can be improved later. But at least we can ship something already useful faster.

@AdamISZ
Copy link
Member

AdamISZ commented Nov 28, 2023

As I said, not the best, but it's already something and can be improved later. But at least we can ship something already useful faster.

I'm OK with that. I guess we can just mention in the release notes that there is a downside with setting a high limit, and that we may in future create a dynamic option. That's better than nothing, indeed.

But to give my opinion: after thinking about it more yesterday, I believe the number-of-blocks target is better. E.g. you could set a default of perhaps 10 or even 12-15 (makers don't care too much about waiting a couple of hours, after all), and then it's up to Takers to choose something not-really-low at the time of their transaction. E.g. they should usually use 3-8, say. The fact there is a possibility of mismatch between each node's feerate for one specific block target probably shouldn't matter too much.

Whereas as a maker I would really be unsure about setting a fixed sats/vb target and leaving it for any length of time, because it could be so wildly wrong in either direction.

@kristapsk
Copy link
Member Author

The fact there is a possibility of mismatch between each node's feerate for one specific block target probably shouldn't matter too much.

I'm not sure about this. I believe we should always announce absolute feerates in JM protocol, regardless is it absolute or relative setting, because of this potential difference.

@AdamISZ
Copy link
Member

AdamISZ commented Dec 2, 2023

I'm hardening my position in favour of block targets.

I'm not sure about this. I believe we should always announce absolute feerates in JM protocol, regardless is it absolute or relative setting, because of this potential difference.

(Nit: using "always" here is a bit confusing; we never announced bitcoin network feerates before. Of course, we did/do announce coinjoin fees both in rel and abs forms, which is different, there is no fluctuation issue there).

Of course, it feels right to always announce absolute values, because it is not ambiguous, but the counterpoint: if you announce block targets, you don't have to keep re-announcing it. It's in my opinion the easiest solution that makes a practically reasonable experience.

Like right now, I go on mempool.space and see 56 s/vb. I set my absolute value to 45, say, in my yg settings, then run the yg and go to lunch/whatever, come back in 12 hours and the fees on the bitcoin network are now more like 150. It can happen in one day, let alone in 5-7 days that you might leave your yg unattended. Now my setting of 45 is basically useless. There is no real solution given the completely unpredictable fee market.

Since Bitcoin Core is working hard to solve that problem in real time, using its mempool, whenever you want to do a transaction it "knows" roughly what fee is needed now, I think it only makes sense to leverage that. To my mind, the problem just isn't solvable without that. We could still announce absolute values, but only if we continually re-announce it (e.g. every 2 minutes, say), using the same backend fee estimator, and adding more code. That is the complex option. I think the right "MVP" for this patch, is the block target one.

@kristapsk
Copy link
Member Author

Since Bitcoin Core is working hard to solve that problem in real time, using its mempool, whenever you want to do a transaction it "knows" roughly what fee is needed now, I think it only makes sense to leverage that.

Bitcoin Core doesn't look at current state of mempool for estimations, it notices time when it first received tx it is relaying, it's feerate and then how many blocks it took for it to confirm. I would not call it "real time". It's really slow adapting to big mempool feerate spikes. https://github.com/bitcoin/bitcoin/blob/714523918ba2b853fc69bee6b04a33ba0c828bf5/src/policy/fees.h#L100

This is the reason some wallets add some other logic on top, for example, Wasabi Wallet can adjust fee upwards for it to fit in top 200 MB of mempool (because 300 MB is the default mempool size limit and you don't want tx to be evicted from mempools too soon).

We could still announce absolute values, but only if we continually re-announce it (e.g. every 2 minutes, say)

Local estimatesmartfee value can change only after each new mined block, no need to re-announce more frequently and also if estimate doesn't change.

@AdamISZ
Copy link
Member

AdamISZ commented Dec 2, 2023

For the first part, those details don't affect the argument, though.

For the second point:

Local estimatesmartfee value can change only after each new mined block, no need to re-announce more frequently and also if estimate doesn't change.

OK, that makes sense and is a good point, but it still stands that it's substantially more work to code that, and (if indeed we go for a once-per-block not once-per-n-minutes) to handle the possible issues around message flooding.

I suppose part of taking your side of the argument would be this: in the long term, we would want a good solution, not a half solution; and that good solution would involve a fee-per-vb not a block target. So the short term solution should agree with that, in format, so that when we make a better version, it's compatible.

@kristapsk
Copy link
Member Author

Did comparision of estimatesmartfee 999 results right now on different mainnet nodes with different settings on different machines. Need to understand, how big could be differences potentially. It looks setting affecting estimates most are configured mempool size limit. Knots v23 has different default OP_RETURN size limit (40 vs 80 bytes), so it will not analyze some transactions.

  • Bitcoin Core v25.1.0, maxmempool=1000 (increases mempool size limit from default 300M to 1G) - 0.00022149
  • Bitcoin Core v25.1.0, maxmempool=300 (default), mempoolfullrbf=1 (enable full-RBF) - 0.00011209
  • Bitcoin Knots v23.0.0, maxmempool=300 (default), mempoolreplacement=fee,optin (Core defaults, disable full-RBF) - 0.00011210
  • Bitcoin Knots v23.0.0, mexmempool=2000 (increases mempool size limit from default 300M to 2G), mempoolreplacement=fee,optin (Core defaults, disable full-RBF) - 0.00022147

@kristapsk
Copy link
Member Author

I suppose part of taking your side of the argument would be this: in the long term, we would want a good solution, not a half solution; and that good solution would involve a fee-per-vb not a block target. So the short term solution should agree with that, in format, so that when we make a better version, it's compatible.

This maybe is not a big problem. Just treat minimum_tx_fee_rate in protocol same way as POLICY.tx_fees in config - values below 1000 are block confirmation targets, values from 1000 and above are sat/kvB values. Makers currently would announce always block targets, but takers could already handle both, don't see a problem there, we have code that handles that already.

@kristapsk
Copy link
Member Author

Rebased

@kristapsk kristapsk force-pushed the minimum-tx-fee-rate branch 3 times, most recently from 789b56a to 96a99ec Compare December 15, 2023 08:50
@kristapsk
Copy link
Member Author

It looks I messed up something with tests again by merging code from different computers together.

@AdamISZ
Copy link
Member

AdamISZ commented Dec 15, 2023

It looks I messed up something with tests again by merging code from different computers together.

I haven't looked at the code diff yet (will do that next), but it seems, from trying locally, that test_daemon_protocol.py now just hangs without doing anything.

@AdamISZ
Copy link
Member

AdamISZ commented Dec 15, 2023

It looks I messed up something with tests again by merging code from different computers together.

I haven't looked at the code diff yet (will do that next), but it seems, from trying locally, that test_daemon_protocol.py now just hangs without doing anything.

Update: it's even a stranger situation.
With pytest ... test/jmbitcoin everything is fine. But for jmclient and jmdaemon the tests just don't start:

(jmvenv) waxwing@here:~/joinmarket-clientserver$pytest --btcpwd=123456abcdef --nirc=2 --btcconf=/home/waxwing/bitcoin.conf -p no:warnings test/jmclient/ -s
============================================================== test session starts ==============================================================
platform linux -- Python 3.8.10, pytest-6.2.5, py-1.11.0, pluggy-1.3.0
rootdir: /home/waxwing/joinmarket-clientserver, configfile: pyproject.toml
plugins: cov-2.5.1
collected 291 items                                                                                                                             

test/jmclient/test_argon2.py ^C

!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! KeyboardInterrupt !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
/usr/lib/python3.8/subprocess.py:1015: KeyboardInterrupt

(and same for jmdaemon - note, the KeyboardInterrupt is me, since nothing was happening)

Even though I have -s, I see nothing printed out. This is very strange, never seen similar behaviour.

Edit: I am seeing this on master too, so probably not relevant to this PR.

@kristapsk
Copy link
Member Author

Main change I did was updating choose_sweep_orders().

@AdamISZ
Copy link
Member

AdamISZ commented Dec 15, 2023

Seems to be hanging when running this RPC command:

bitcoin-cli -regtest -rpcuser=bitcoinrpc -rpcpassword=123456abcdef -rpcwait -named createwallet wallet_name=jm-test-wallet descriptors=false

at least, as of current master branch. My bitcoind is v26.0. I can do more testing in a little while, just thought I should share what I saw so far.

@kristapsk
Copy link
Member Author

at least, as of current master branch. My bitcoind is v26.0.

#1619 should solve that.

@kristapsk
Copy link
Member Author

Or just add deprecatedrpc=create_bdb to test/bitcoin.conf manually for v26.

@AdamISZ
Copy link
Member

AdamISZ commented Dec 15, 2023

Or just add deprecatedrpc=create_bdb to test/bitcoin.conf manually for v26.

I can't figure out why but it's still blocking in the same place after this change. Again, I am currently testing on master. Did I forget something?

Edit: and just to confirm it works fine on Core v25.0

@AdamISZ
Copy link
Member

AdamISZ commented Dec 16, 2023

Or just add deprecatedrpc=create_bdb to test/bitcoin.conf manually for v26.

I can't figure out why but it's still blocking in the same place after this change. Again, I am currently testing on master. Did I forget something?

Edit: and just to confirm it works fine on Core v25.0

See #1619 (comment)

@AdamISZ
Copy link
Member

AdamISZ commented Dec 16, 2023

Unfortunately, after spending a couple of hours yesterday solving the above problem, I've now lost another couple of hours today battling with our test setup as per my messages on Telegram.

Basically I can't get the pytest invocation to even recognize what code it should be running. It sometimes is taking the packages (jmdaemon, jmclient etc) code from build/lib, other times from jmvenv/lib/.../site-packages/, and even from ~/.local/lib (?), after multiple attempts to reinstall with ./install.sh I seem to be getting nowhere. So right now I can't seem to test a PR properly ...

@AdamISZ
Copy link
Member

AdamISZ commented Dec 18, 2023

@kristapsk ok now I've fixed my problems with tests (I need to manually pip install -e for some weird reason), I can see the error and correct it: you need to add u'minimum_tx_fee_rate': 0 entries to t_orderbook in test/jmdaemon/msgdata.py.

Unfortunate that this is a data duplication with the same thing in jmclient tests, but they're supposed to be completely independent packages, so, I guess it's fine anyway.

@@ -352,14 +365,18 @@ def calc_zero_change_cj_amount(ordercombo):
cjamount = int(cjamount.quantize(Decimal(1)))
return cjamount, int(sumabsfee + sumrelfee * cjamount)

fee_per_kb = jm_single().bc_interface.estimate_fee_per_kb(
jm_single().config.getint("POLICY", "tx_fees"), randomize=False)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an argument for throwing in a 20% tolerance here, i.e. if 2 participants are trying to use roughly the same fee estimate (e.g. 10 blocks target), and just happen to be slightly different, then let's make it more likely that they'll succeed to negotiate?

@AdamISZ
Copy link
Member

AdamISZ commented Dec 21, 2023

In testing out some simple scenarios, I realised the following addition is very logical:

in jmclient.support.choose_orders, before filtering on minimum_tx_fee_rate, add:

# provide a message to the user to let them know that there are offers not
# available due to too-low feerate:
orders_fee_too_low = [o for o in orders if o["minimum_tx_fee_rate"] > fee_per_kb]
#Filter those not accepting our tx feerate
orders = [o for o in orders if o["minimum_tx_fee_rate"] <= fee_per_kb]

and then at the end, if choosing has failed you can add more info to the warning message:

    if num_counterparties > len(counterparties):
        log.warn(('ERROR not enough liquidity in the orderbook n=%d '
                   'suitable-counterparties=%d amount=%d totalorders=%d') %
                  (num_counterparties, len(counterparties), cj_amount,
                   len(orders_fees)))
        if len(orders_fee_too_low) > 0:
            log.warn(("Note that these counterparty offers were rejected "
                      "because they demanded too high a transaction fee: "
                      "{}".format(orders_fee_too_low)))

(Not a perfect implementation of the idea, but - it makes a lot of sense I think. Also this doesn't address the non CLI case).

@kristapsk
Copy link
Member Author

and then at the end, if choosing has failed you can add more info to the warning message

Second message could have a lots of output in some cases, I think warning could just output number of counterparties filtered out and full list should be debug message.

@kristapsk
Copy link
Member Author

warning could just output number of counterparties filtered out

And also their minimum tx fee rate range.

@AdamISZ
Copy link
Member

AdamISZ commented Dec 24, 2023

and then at the end, if choosing has failed you can add more info to the warning message

Second message could have a lots of output in some cases, I think warning could just output number of counterparties filtered out and full list should be debug message.

Agreed.

…not available due to too low feerate

Co-authored-by: Adam Gibson <[email protected]>
@AdamISZ
Copy link
Member

AdamISZ commented Dec 29, 2023

dee7e61 looks good, thanks.

@kristapsk
Copy link
Member Author

    if num_counterparties > len(counterparties):
        log.warn(('ERROR not enough liquidity in the orderbook n=%d '
                   'suitable-counterparties=%d amount=%d totalorders=%d') %
                  (num_counterparties, len(counterparties), cj_amount,
                   len(orders_fees)))

While doing some signet testing I understood that this message is useful even without this PR (there was no liqudity in orderbook at all).

@kristapsk
Copy link
Member Author

    if num_counterparties > len(counterparties):
        log.warn(('ERROR not enough liquidity in the orderbook n=%d '
                   'suitable-counterparties=%d amount=%d totalorders=%d') %
                  (num_counterparties, len(counterparties), cj_amount,
                   len(orders_fees)))

While doing some signet testing I understood that this message is useful even without this PR (there was no liqudity in orderbook at all).

I mean displaying more information even without minimum tx fee rate filters.

@kristapsk
Copy link
Member Author

Another thing that needs to be handled - as a taker manual txfee in sats/kvB can be specified. When choosing makers, that needs to be matched against confirmation targets.

Yes that's a good point. We would need a reverse of estimate_fee_per_kb, i.e. a function that goes from blocks target -> sats/kvb.

Hmm this is a pretty convincing argument that the process is too complex. But it seems, every time I think a solution through in detail, it always has extra complexity. The main factors are: we have two different ways of specifying network fees; the feerates are extremely volatile; the conversion of a target blocks to a feerate is not objective/reliable (varies between participants); re-announcing very frequently is problematic for privacy and network traffic.

There is another thing - nothing in JoinMarket protocol enforces participiants to use Bitcoin Core as a fee estimator. Yes, JM currently always does that, but other clients might not or people could patch software to change estimator, see #1664. And protocols should be designed by future in mind too, not only today. So putting any relative fees into orderbook / protocol seems totally wrong to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
High priority protocol related to the communication between maker / taker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants