-
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
Draft
kristapsk
wants to merge
2
commits into
JoinMarket-Org:master
Choose a base branch
from
kristapsk:minimum-tx-fee-rate
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,15 @@ | ||
from functools import reduce | ||
import random | ||
from jmbase.support import get_log | ||
from decimal import Decimal | ||
from .configure import get_bondless_makers_allowance | ||
|
||
from functools import reduce | ||
from math import exp | ||
from typing import Callable, List, Optional, Tuple | ||
|
||
from jmbase.support import get_log | ||
from .configure import get_bondless_makers_allowance, jm_single | ||
|
||
|
||
ORDER_KEYS = ['counterparty', 'oid', 'ordertype', 'minsize', 'maxsize', 'txfee', | ||
'cjfee'] | ||
'cjfee', 'minimum_tx_fee_rate'] | ||
|
||
log = get_log() | ||
|
||
|
@@ -45,7 +47,7 @@ def rand_exp_array(lamda, n): | |
return [random.expovariate(1.0 / lamda) for _ in range(n)] | ||
|
||
|
||
def rand_weighted_choice(n, p_arr): | ||
def rand_weighted_choice(n: int, p_arr: list) -> int: | ||
""" | ||
Choose a value in 0..n-1 | ||
with the choice weighted by the probabilities | ||
|
@@ -177,7 +179,7 @@ def calc_cj_fee(ordertype, cjfee, cj_amount): | |
return real_cjfee | ||
|
||
|
||
def weighted_order_choose(orders, n): | ||
def weighted_order_choose(orders: List[dict], n: int) -> dict: | ||
""" | ||
Algorithm for choosing the weighting function | ||
it is an exponential | ||
|
@@ -208,18 +210,18 @@ def weighted_order_choose(orders, n): | |
return orders[chosen_order_index] | ||
|
||
|
||
def random_under_max_order_choose(orders, n): | ||
def random_under_max_order_choose(orders: List[dict], n: int) -> dict: | ||
# orders are already pre-filtered for max_cj_fee | ||
return random.choice(orders) | ||
|
||
|
||
def cheapest_order_choose(orders, n): | ||
def cheapest_order_choose(orders: List[dict], n: int) -> dict: | ||
""" | ||
Return the cheapest order from the orders. | ||
""" | ||
return orders[0] | ||
|
||
def fidelity_bond_weighted_order_choose(orders, n): | ||
def fidelity_bond_weighted_order_choose(orders: List[dict], n: int) -> dict: | ||
""" | ||
choose orders based on fidelity bond for improved sybil resistance | ||
|
@@ -247,19 +249,28 @@ def check_max_fee(fee): | |
return check_max_fee | ||
|
||
|
||
def choose_orders(offers, cj_amount, n, chooseOrdersBy, ignored_makers=None, | ||
pick=False, allowed_types=["sw0reloffer", "sw0absoffer"], | ||
max_cj_fee=(1, float('inf'))): | ||
def choose_orders(offers: List[dict], | ||
cj_amount: int, | ||
num_counterparties: int, | ||
chooseOrdersBy: Callable[[List[dict], int], dict], | ||
ignored_makers: Optional[List[str]] = None, | ||
pick: bool = False, | ||
allowed_types: List[str] = ["sw0reloffer", "sw0absoffer"], | ||
max_cj_fee: Tuple = (1, float('inf'))) -> Tuple[Optional[dict], int]: | ||
is_within_max_limits = _get_is_within_max_limits( | ||
max_cj_fee[0], max_cj_fee[1], cj_amount) | ||
if ignored_makers is None: | ||
ignored_makers = [] | ||
fee_per_kb = jm_single().bc_interface.estimate_fee_per_kb( | ||
jm_single().config.getint("POLICY", "tx_fees"), randomize=False) | ||
#Filter ignored makers and inappropriate amounts | ||
orders = [o for o in offers if o['counterparty'] not in ignored_makers] | ||
orders = [o for o in orders if o['minsize'] < cj_amount] | ||
orders = [o for o in orders if o['maxsize'] > cj_amount] | ||
#Filter those not using wished-for offertypes | ||
orders = [o for o in orders if o["ordertype"] in allowed_types] | ||
#Filter those not accepting our tx feerate | ||
AdamISZ marked this conversation as resolved.
Show resolved
Hide resolved
|
||
orders = [o for o in orders if o["minimum_tx_fee_rate"] <= fee_per_kb] | ||
|
||
orders_fees = [] | ||
for o in orders: | ||
|
@@ -268,10 +279,11 @@ def choose_orders(offers, cj_amount, n, chooseOrdersBy, ignored_makers=None, | |
orders_fees.append((o, fee)) | ||
|
||
counterparties = set(o['counterparty'] for o, f in orders_fees) | ||
if n > len(counterparties): | ||
if num_counterparties > len(counterparties): | ||
log.warn(('ERROR not enough liquidity in the orderbook n=%d ' | ||
'suitable-counterparties=%d amount=%d totalorders=%d') % | ||
(n, len(counterparties), cj_amount, len(orders_fees))) | ||
(num_counterparties, len(counterparties), cj_amount, | ||
len(orders_fees))) | ||
# TODO handle not enough liquidity better, maybe an Exception | ||
return None, 0 | ||
""" | ||
|
@@ -294,8 +306,9 @@ def choose_orders(offers, cj_amount, n, chooseOrdersBy, ignored_makers=None, | |
])) | ||
total_cj_fee = 0 | ||
chosen_orders = [] | ||
for i in range(n): | ||
chosen_order, chosen_fee = chooseOrdersBy(orders_fees, n) | ||
for i in range(num_counterparties): | ||
chosen_order, chosen_fee = chooseOrdersBy(orders_fees, | ||
num_counterparties) | ||
# remove all orders from that same counterparty | ||
# only needed if offers are manually picked | ||
orders_fees = [o | ||
|
@@ -308,14 +321,14 @@ def choose_orders(offers, cj_amount, n, chooseOrdersBy, ignored_makers=None, | |
return result, total_cj_fee | ||
|
||
|
||
def choose_sweep_orders(offers, | ||
total_input_value, | ||
total_txfee, | ||
n, | ||
chooseOrdersBy, | ||
ignored_makers=None, | ||
allowed_types=['sw0reloffer', 'sw0absoffer'], | ||
max_cj_fee=(1, float('inf'))): | ||
def choose_sweep_orders(offers: List[dict], | ||
total_input_value: int, | ||
total_txfee: int, | ||
num_counterparties: int, | ||
chooseOrdersBy: Callable[[List[dict], int], dict], | ||
ignored_makers: Optional[List[str]] = None, | ||
allowed_types: List[str] = ['sw0reloffer', 'sw0absoffer'], | ||
max_cj_fee: Tuple = (1, float('inf'))) -> Tuple[Optional[dict], int, int]: | ||
""" | ||
choose an order given that we want to be left with no change | ||
i.e. sweep an entire group of utxos | ||
|
@@ -332,7 +345,7 @@ def choose_sweep_orders(offers, | |
if ignored_makers is None: | ||
ignored_makers = [] | ||
|
||
def calc_zero_change_cj_amount(ordercombo): | ||
def calc_zero_change_cj_amount(ordercombo: List[dict]) -> Tuple[int, int]: | ||
sumabsfee = 0 | ||
sumrelfee = Decimal('0') | ||
sumtxfee_contribution = 0 | ||
|
@@ -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 commentThe 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? |
||
log.debug('choosing sweep orders for total_input_value = ' + str( | ||
total_input_value) + ' n=' + str(n)) | ||
total_input_value) + ' n=' + str(num_counterparties)) | ||
offers = [o for o in offers if o["ordertype"] in allowed_types] | ||
#Filter ignored makers and inappropriate amounts | ||
offers = [o for o in offers if o['counterparty'] not in ignored_makers] | ||
offers = [o for o in offers if o['minsize'] < total_input_value] | ||
# while we do not know the exact cj value yet, we can approximate a ceiling: | ||
offers = [o for o in offers if o['maxsize'] > (total_input_value - total_txfee)] | ||
#Filter those not accepting our tx feerate | ||
offers = [o for o in offers if o["minimum_tx_fee_rate"] <= fee_per_kb] | ||
|
||
log.debug('orderlist = \n' + '\n'.join([str(o) for o in offers])) | ||
orders_fees = [(o, calc_cj_fee(o['ordertype'], o['cjfee'], | ||
|
@@ -376,13 +393,14 @@ def calc_zero_change_cj_amount(ordercombo): | |
if is_within_max_limits(v[1])).values(), | ||
key=feekey) | ||
chosen_orders = [] | ||
while len(chosen_orders) < n: | ||
for i in range(n - len(chosen_orders)): | ||
if len(orders_fees) < n - len(chosen_orders): | ||
while len(chosen_orders) < num_counterparties: | ||
for i in range(num_counterparties - len(chosen_orders)): | ||
if len(orders_fees) < num_counterparties - len(chosen_orders): | ||
log.debug('ERROR not enough liquidity in the orderbook') | ||
# TODO handle not enough liquidity better, maybe an Exception | ||
return None, 0, 0 | ||
chosen_order, chosen_fee = chooseOrdersBy(orders_fees, n) | ||
chosen_order, chosen_fee = chooseOrdersBy(orders_fees, | ||
num_counterparties) | ||
log.debug('chosen = ' + str(chosen_order)) | ||
# remove all orders from that same counterparty | ||
orders_fees = [ | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 tosrc.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.
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 foron_tx_unconfirmed
oron_tx_confirmed
.This "extra code" is an extra communication type specified in
jmbase.commands.py
and then extra functions to send/receive message injmdaemon.daemon_protocol.py
andjmclient.client_protocol.py
. If we don't do that, then as mentioned above, the daemon just reads the current offer from an internally storedofferlist
variable, and doesn't know that anything changed injmclient
.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.
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()
.