-
Notifications
You must be signed in to change notification settings - Fork 179
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
base: master
Are you sure you want to change the base?
Conversation
@@ -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 |
There was a problem hiding this comment.
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 :) ).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()
.
So it seems like the only issue we have to get fixed on is as per your original description:
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 There's a nuance, in that scenario, which should be noted: Case 1: the taker with the new code: since the flow is 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. |
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) |
Should |
Hmm, it's not clear to me, but I see what you're saying. Here, if we take the option of dynamically updating the If instead, we code it to update the 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 |
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 |
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. |
…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
So my best suggestion is: 1/ don't update JM_VERSION 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. |
I am first focusing on fixing current test failures (that 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 It almost seems like we should do the opposite of this PR: in the maker's offer, only allow a 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? |
Maybe we could have the maker's initial value (say, 20k s/kvb) and multiply it proportionately by the changes in the output of That just seems too confusing/complicated/janky. |
Test causing |
I'm not sure if it's just not showing up clearly in the CI test because of the absence of the
then I get the error on the terminal:
which should be pretty easy to fix. |
Indeed the following patch fixed those errors in 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") |
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.
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. |
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. |
I'm hardening my position in favour of block targets.
(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. |
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).
Local |
For the first part, those details don't affect the argument, though. For the second point:
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. |
Did comparision of
|
This maybe is not a big problem. Just treat |
ca972cd
to
071e362
Compare
Rebased |
789b56a
to
96a99ec
Compare
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 |
Update: it's even a stranger situation.
(and same for Even though I have Edit: I am seeing this on master too, so probably not relevant to this PR. |
Main change I did was updating |
Seems to be hanging when running this RPC command:
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. |
#1619 should solve that. |
Or just add |
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) |
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 |
@kristapsk ok now I've fixed my problems with tests (I need to manually Unfortunate that this is a data duplication with the same thing in |
96a99ec
to
e5bfcf2
Compare
e5bfcf2
to
43abe46
Compare
@@ -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) |
There was a problem hiding this comment.
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?
In testing out some simple scenarios, I realised the following addition is very logical: in # 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). |
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. |
And also their minimum tx fee rate range. |
Agreed. |
…not available due to too low feerate Co-authored-by: Adam Gibson <[email protected]>
dee7e61 looks good, thanks. |
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. |
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. |
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.