-
Notifications
You must be signed in to change notification settings - Fork 178
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
Move YG settings to a config file #727
Conversation
Some migration docs about this might be needed. Or @AdamISZ will just add this to release notes? |
Also, is there a need for a separate config file, can't these just be added to joinmarket.cfg under separate section (like |
Adding to the existing config file is probably a good idea. I just didn't want to write any code that touches other parts since I'm mostly a spectator/user and not a coder/developer. Don't think there has to be any migration docs since the current user experience is the same with the settings going back to the defaults whenever the script is updated. |
Separate vs new config file is something that occurred to me too. I'm easy on that; one might vaguely imagine a nice thing about them being separate is moving that config separately between setups, but .. that's not really too much of an argument. The existing config is getting very big, but no big deal I guess, considering that users always have to edit it when they first start, anyway. |
This seems simplest to me, and as someone who sometimes puts up offers using abs, and other times using rel, it would be nice to be able to stipulate params in the config for both absoffer and reloffer, have them be persistent. Then add a flag in the start command to stipulate which is to be used....i.e: "python yg-privacyenhanced.py -abs wallet.jmdat" or "python yg-privacyenhanced.py -rel wallet.jmdat" |
Maybe overkill and definitely out of scope of this PR, but probably adding generic support for overriding configuration settings with command line arguments then is better idea? |
Yeah pretty much agreed there, it would be in line with other things to have all these settings optionally overridden by command line .. I don't mind if that's included in this PR or not. |
I've updated the PR to use existing I would suggest a separate PR to add command line options. |
scripts/yg-privacyenhanced.py
Outdated
|
||
# end of settings customization | ||
config = ConfigParser() | ||
config.read(os.path.join(lookup_appdata_folder(jm_single().APPNAME),"joinmarket.cfg")) |
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 jm_single().config
(jm_single
is imported from jmclient
in yieldgenerator.py
, which is where the start up function ygmain()
is called; the config is set after load_program_config
has been called) is accessible, this processing is not needed:
Instead, you would just add the defaults to the defaultconfig string here:
joinmarket-clientserver/jmclient/jmclient/configure.py
Lines 94 to 352 in 14f0f46
defaultconfig = \ | |
""" | |
[DAEMON] | |
#set to 1 to run the daemon service within this process; | |
#set to 0 if the daemon is run separately (using script joinmarketd.py) | |
no_daemon = 1 | |
#port on which daemon serves; note that communication still | |
#occurs over this port even if no_daemon = 1 | |
daemon_port = 27183 | |
#currently, running the daemon on a remote host is | |
#*NOT* supported, so don't change this variable | |
daemon_host = localhost | |
#by default the client-daemon connection is plaintext, set to 'true' to use TLS; | |
#for this, you need to have a valid (self-signed) certificate installed | |
use_ssl = false | |
[BLOCKCHAIN] | |
# options: bitcoin-rpc, regtest, bitcoin-rpc-no-history, no-blockchain | |
# When using bitcoin-rpc-no-history remember to increase the gap limit to scan for more addresses, try -g 5000 | |
# Use 'no-blockchain' to run the ob-watcher.py script in scripts/obwatch without current access | |
# to Bitcoin Core; note that use of this option for any other purpose is currently unsupported. | |
blockchain_source = bitcoin-rpc | |
# options: testnet, mainnet | |
# Note: for regtest, use network = testnet | |
network = mainnet | |
rpc_host = localhost | |
rpc_port = 8332 | |
rpc_user = bitcoin | |
rpc_password = password | |
rpc_wallet_file = | |
[MESSAGING:server1] | |
host = irc.darkscience.net | |
channel = joinmarket-pit | |
port = 6697 | |
usessl = true | |
socks5 = false | |
socks5_host = localhost | |
socks5_port = 9050 | |
#for tor | |
#host = darksci3bfoka7tw.onion | |
#socks5 = true | |
[MESSAGING:server2] | |
host = irc.hackint.org | |
channel = joinmarket-pit | |
port = 6697 | |
usessl = true | |
socks5 = false | |
socks5_host = localhost | |
socks5_port = 9050 | |
#for tor | |
#host = ncwkrwxpq2ikcngxq3dy2xctuheniggtqeibvgofixpzvrwpa77tozqd.onion | |
#port = 6667 | |
#usessl = false | |
#socks5 = true | |
#Agora sometimes seems to be unreliable. Not active by default for that reason. | |
#[MESSAGING:server3] | |
#host = agora.anarplex.net | |
#channel = joinmarket-pit | |
#port = 14716 | |
#usessl = true | |
#socks5 = false | |
#socks5_host = localhost | |
#socks5_port = 9050 | |
# | |
##for tor | |
##host = cfyfz6afpgfeirst.onion | |
##port = 6667 | |
##usessl = false | |
##socks5 = true | |
[LOGGING] | |
# Set the log level for the output to the terminal/console | |
# Possible choices: DEBUG / INFO / WARNING / ERROR | |
# Log level for the files in the logs-folder will always be DEBUG | |
console_log_level = INFO | |
# Use color-coded log messages to help distinguish log levels?: | |
color = true | |
[TIMEOUT] | |
maker_timeout_sec = 60 | |
unconfirm_timeout_sec = 180 | |
confirm_timeout_hours = 6 | |
[POLICY] | |
# Use segwit style wallets and transactions | |
# Only set to false for old wallets, Joinmarket is now segwit only. | |
segwit = true | |
# Use native segwit (bech32) wallet. If set to false, p2sh-p2wkh | |
# will be used when generating the addresses for this wallet. | |
# Notes: 1. The default joinmarket pit is native segwit. | |
# 2. You cannot change the type of a pre-existing wallet. | |
native = true | |
# for dust sweeping, try merge_algorithm = gradual | |
# for more rapid dust sweeping, try merge_algorithm = greedy | |
# for most rapid dust sweeping, try merge_algorithm = greediest | |
# but don't forget to bump your miner fees! | |
merge_algorithm = default | |
# The fee estimate is based on a projection of how many satoshis | |
# per kB are needed to get in one of the next N blocks, N set here | |
# as the value of 'tx_fees'. This cost estimate is high if you set | |
# N=1, so we choose 3 for a more reasonable figure, as our default. | |
# You can also set your own fee/kb: any number higher than 1000 will | |
# be interpreted as the fee in satoshi per kB that you wish to use | |
# example: N=30000 will use 30000 sat/kB as a fee, while N=5 | |
# will use the estimate from your selected blockchain source | |
# Note that there will be a 20% variation around any manually chosen | |
# values, so if you set N=10000, it might use any value between | |
# 8000 and 12000 for your transactions. | |
tx_fees = 3 | |
# For users getting transaction fee estimates over an API, | |
# place a sanity check limit on the satoshis-per-kB to be paid. | |
# This limit is also applied to users using Core, even though | |
# Core has its own sanity check limit, which is currently | |
# 1,000,000 satoshis. | |
absurd_fee_per_kb = 350000 | |
# Maximum absolute coinjoin fee in satoshi to pay to a single | |
# market maker for a transaction. Both the limits given in | |
# max_cj_fee_abs and max_cj_fee_rel must be exceeded in order | |
# to not consider a certain offer. | |
#max_cj_fee_abs = x | |
# Maximum relative coinjoin fee, in fractions of the coinjoin value | |
# e.g. if your coinjoin amount is 2 btc (200000000 satoshi) and | |
# max_cj_fee_rel = 0.001 (0.1%), the maximum fee allowed would | |
# be 0.002 btc (200000 satoshi) | |
#max_cj_fee_rel = x | |
# the range of confirmations passed to the `listunspent` bitcoind RPC call | |
# 1st value is the inclusive minimum, defaults to one confirmation | |
# 2nd value is the exclusive maximum, defaults to most-positive-bignum (Google Me!) | |
# leaving it unset or empty defers to bitcoind's default values, ie [1, 9999999] | |
#listunspent_args = [] | |
# that's what you should do, unless you have a specific reason, eg: | |
# !!! WARNING !!! CONFIGURING THIS WHILE TAKING LIQUIDITY FROM | |
# !!! WARNING !!! THE PUBLIC ORDERBOOK LEAKS YOUR INPUT MERGES | |
# spend from unconfirmed transactions: listunspent_args = [0] | |
# display only unconfirmed transactions: listunspent_args = [0, 1] | |
# defend against small reorganizations: listunspent_args = [3] | |
# who is at risk of reorganization?: listunspent_args = [0, 2] | |
# NB: using 0 for the 1st value with scripts other than wallet-tool could cause | |
# spends from unconfirmed inputs, which may then get malleated or double-spent! | |
# other counterparties are likely to reject unconfirmed inputs... don't do it. | |
# tx_broadcast: options: self, random-peer, not-self. | |
# | |
# self = broadcast transaction with your own bitcoin node. | |
# | |
# random-peer = everyone who took part in the coinjoin has a chance of broadcasting | |
# note: if your counterparties do not support it, you will fall back | |
# to broadcasting via your own node. | |
# | |
# not-self = never broadcast with your own bitcoin node. | |
# note: in this case if your counterparties do not broadcast for you, you | |
# will have to broadcast the tx manually (you can take the tx hex from the log | |
# or terminal) via some other channel. It is not recommended to choose this | |
# option when running schedules/tumbler. | |
tx_broadcast = self | |
# If makers do not respond while creating a coinjoin transaction, | |
# the non-responding ones will be ignored. This is the minimum | |
# amount of makers which we are content with for the coinjoin to | |
# succceed. Less makers means that the whole process will restart | |
# after a timeout. | |
minimum_makers = 4 | |
# Threshold number of satoshis below which an incoming utxo | |
# to a reused address in the wallet will be AUTOMATICALLY frozen. | |
# This avoids forced address reuse attacks; see: | |
# https://en.bitcoin.it/wiki/Privacy#Forced_address_reuse | |
# | |
# The default is to ALWAYS freeze a utxo to an already used address, | |
# whatever the value of it, and this is set with the value -1. | |
max_sats_freeze_reuse = -1 | |
############################## | |
#THE FOLLOWING SETTINGS ARE REQUIRED TO DEFEND AGAINST SNOOPERS. | |
#DON'T ALTER THEM UNLESS YOU UNDERSTAND THE IMPLICATIONS. | |
############################## | |
# number of retries allowed for a specific utxo, to prevent DOS/snooping. | |
# Lower settings make snooping more expensive, but also prevent honest users | |
# from retrying if an error occurs. | |
taker_utxo_retries = 3 | |
# number of confirmations required for the commitment utxo mentioned above. | |
# this effectively rate-limits a snooper. | |
taker_utxo_age = 5 | |
# percentage of coinjoin amount that the commitment utxo must have | |
# as a minimum BTC amount. Thus 20 means a 1BTC coinjoin requires the | |
# utxo to be at least 0.2 btc. | |
taker_utxo_amtpercent = 20 | |
#Set to 1 to accept broadcast PoDLE commitments from other bots, and | |
#add them to your blacklist (only relevant for Makers). | |
#There is no way to spoof these values, so the only "risk" is that | |
#someone fills your blacklist file with a lot of data. | |
accept_commitment_broadcasts = 1 | |
#Location of your commitments.json file (stores commitments you've used | |
#and those you want to use in future), relative to the scripts directory. | |
commit_file_location = cmtdata/commitments.json | |
[PAYJOIN] | |
# for the majority of situations, the defaults | |
# need not be altered - they will ensure you don't pay | |
# a significantly higher fee. | |
# MODIFICATION OF THESE SETTINGS IS DISADVISED. | |
# Payjoin protocol version; currently only '1' is supported. | |
payjoin_version = 1 | |
# servers can change their destination address by default (0). | |
# if '1', they cannot. Note that servers can explicitly request | |
# that this is activated, in which case we respect that choice. | |
disable_output_substitution = 0 | |
# "default" here indicates that we will allow the receiver to | |
# increase the fee we pay by: | |
# 1.2 * (our_fee_rate_per_vbyte * vsize_of_our_input_type) | |
# (see https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#span_idfeeoutputspanFee_output) | |
# (and 1.2 to give breathing room) | |
# which indicates we are allowing roughly one extra input's fee. | |
# If it is instead set to an integer, then that many satoshis are allowed. | |
# Additionally, note that we will also set the parameter additionafeeoutputindex | |
# to that of our change output, unless there is none in which case this is disabled. | |
max_additional_fee_contribution = default | |
# this is the minimum satoshis per vbyte we allow in the payjoin | |
# transaction; note it is decimal, not integer. | |
min_fee_rate = 1.1 | |
# for payjoins to hidden service endpoints, the socks5 configuration: | |
onion_socks5_host = localhost | |
onion_socks5_port = 9050 | |
# for payjoin onion service creation, the tor control configuration: | |
tor_control_host = localhost | |
# or, to use a UNIX socket | |
# control_host = unix:/var/run/tor/control | |
tor_control_port = 9051 | |
# in some exceptional case the HS may be SSL configured, | |
# this feature is not yet implemented in code, but here for the | |
# future: | |
hidden_service_ssl = false | |
""" |
because the config is read in a way that automatically uses the defaults, if there is no setting in the file <datadir>/joinmarket.cfg
.
There's something else that I forgot in my previous comment about command line args. If you look at the actual arguments of the yield generator scripts (with joinmarket-clientserver/jmclient/jmclient/yieldgenerator.py Lines 197 to 217 in 14f0f46
... it already has all the settings for the basic yield generator. Therefore I think we can just add txfee_factor and cjfee_factor for the extra randomness of yg-privacyenhanced.py. I can do that after we merge this.
|
So as a result of those two things, now I think about it, the simplest is: (1) In (2) add the remaining (3) Add the (4) Remove the settings (and the arguments in the call to That should be all. Notice I'm folding in command line args because they were already folded in, in the current code. |
After discussion on IRC, added a substantial rewrite in 354d37f , in line with the previous comment. |
I have some test suite failures with this.
|
Thanks for reminder, YG class args changed, so those files needed an update, test suite passes here as of 7574979 |
Can confirm, test suite passes. But now got error when starting
|
Ah indeed, a bug I didn't spot because I didn't trigger that error condition. Thanks, fix incoming. |
I don't get now from where it gets this default minsize value if I don't have yg settings section in joinmarket.cfg.
Shouldn't it be 100000? |
Also, |
Try it with --ordertype=absoffer, or alternatively --txfee=0, because otherwise it changes quite a lot depending on your fee, remember it calculates a minsize so that you always profit. (these things are not changed, of course, talking about existing algorithms) |
Yeah I can reproduce weird behavior like you say, on regtest, with defaults and no entries in joinmarket.cfg, just setting |
@kristapsk i finally figured out the reason there is a delta between master and this branch, it's a bug that exists in master, see #733 which fixes it. |
I think this is now in a good and useful state, but pushing it back to 0.8.1 to avoid adding too much cognitive load to the new release, unless anyone very strongly disagrees :) |
will need to update the default txfee to 0 based on #733 my only reason to add this to 0.8 is to avoid another |
Right, but this problem is less of a problem than you'd imagine, and we regularly upgrade the config file because of this trick: on startup, the application first loads the text config in |
I think it's okay to wait for after 0.8.0 for this, as migrating to bech32 orderbook by default will be a pretty big change by itself. Also, I see this as a blocker for #487, but don't think it's realistic to have that one for 0.8.0 either. |
Rebased on master. Will need some more testing. |
@openoms your feedback on the current state of this would be particularly valuable since you're the main person outside the jm devs who actually "packages" the install and instructs people how to use it. this would require a change in instructions on how to set up the yieldgen. |
@AdamISZ I think the changes make sense. |
Rebased on master, and squashed. (github may erroneously show multiple commits there, but there is now only 1). |
Tests conducted:
(in these I was mainly checking that the yieldgenerator offer strings looked correct according to the settings, but I also sanity checked by running sendpayment against those ygs). Since all these tests pass, I'm ready to merge (will add one more small commit that's just a slight modification to the ygrunner test script), after squashing. But will wait for @kristapsk as I think he's looking at/testing it now, also. |
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.
ACK b21d810 (did some manual testing with yield generator with and without new section added to joinmarket.cfg in addition to checking that test suite still passes)
But should not forget to add to release notes that there is now |
Good catch. I will add that error message, because agreed it would be bad if it silently got ignored. |
It turns out we don't need to add anything here, @kristapsk ; optparse deals with it appropriately due to ambiguity:
... and quits immediately. |
Allow YG settings to be saved to joinmarket.cfg. Before this commit, yield generator settings were set inside the python script, which is not good. After this commit, the order of precedence for the settings for a yield generator is: * Command line arguments, or * Settings in [YIELDGENERATOR] in joinmarket.cfg, or * default config settings in jmclient.configure.py
Squashed, will now merge. |
603d3fc Move YG settings to a config file (Kristaps Kaupe) Pull request description: JoinMarket-Org/joinmarket-clientserver#727 Top commit has no ACKs. Tree-SHA512: 7876ca5d0813b7e600689b35e366faf96f7fa4cf0bcb65213ebe11183d934cbf26f2b157737147f088e6948b2fcfbc0815bd750992795e8721d73377ec8b9106
Allow YG settings to be saved to yg.cfg config file (same location as the joinmarket.cfg file). The update will also create the cfg file with the default settings if the file does not exist.