[WIP] Make arguments reconfigurable at runtime via RPC #7289

pull luke-jr wants to merge 1 commits into bitcoin:master from luke-jr:rpc_setarg changing 6 files +74 −9

luke-jr commented at 5:13 pm on January 4, 2016: member

Just looking for concept ACK/NACK on this before proceeding. (Currently untested, but compiles.)

Maybe tips on how to avoid void* and ugly casting too…
[WIP] Make arguments reconfigurable at runtime via RPC dbe2d8b17e
MarcoFalke commented at 5:50 pm on January 4, 2016: member

Concept ACK. Clearly makes sense for some args. (c.f. setmocktime)

Not sure if this should be enabled for all args. (Setting minRelayTxFee may result in unexpected behavior.)
jgarzik commented at 5:55 pm on January 4, 2016: contributor

Rough concept ACK.

To speak generally and scope the problem, each setting has been written with the idea that it is largely static throughout program runtime.

At an engineering level, you have to revisit each setting in the context of thread safety, as well as wider impacts such as, e.g. changing min-relay-tx-fee and then running with a mempool temporarily disjoint from that value (or, alternately, adding setting-specific trigger code to be executed when setting A or B is changed at runtime).

tl;dr Good goal, but requires per-setting analysis, and possibly per-setting runtime reconfiguration code.
jonasschnelli commented at 7:47 pm on January 4, 2016: contributor

Concept ACK. Agree with @jgarzik.

Currently, your new setarg command only protects the variable by cs_main (and cs_wallet). Not sure if that would be sufficient (IMO it wouldn’t be sufficient for net limits (-maxuploadtarget)). Instead of directly accessing globals vars, a setter/getter (could also be global) in the according code region would result in a better, cleaner code (or a class that could handle the global vars).

Conceptual, when supporting a setarg, there should also be something like a getarg (to check/ensure a certain runtime var).
jonasschnelli added the label Refactoring on Jan 4, 2016
jonasschnelli added the label RPC on Jan 4, 2016
paveljanik commented at 8:00 pm on January 4, 2016: contributor

This would be great!
luke-jr commented at 9:28 pm on January 4, 2016: member

Yes, I intentionally designed this to require explicit opt-in from each setting.

Having a getter/setter for each item would IMO bloat the code and discourage making options configurable instead of making it trivial to do.

If we’re replacing settings with classes (is that really low-overhead enough for some settings?), however, that might solve all the concerns at once - it could manage the correct locks only, and refuse assignment after being read (for arguments that’s appropriate for).
GIJensen commented at 1:33 am on January 19, 2016: none

Concept ACK. I know this could save me from a few restarts here and there :).

in src/init.cpp: in dbe2d8b17e

989+    RegisterArg(fIsBareMultisigStd, "permitbaremultisig", DEFAULT_PERMIT_BAREMULTISIG);
990+    RegisterArg(fAcceptDatacarrier, "datacarrier", DEFAULT_ACCEPT_DATACARRIER);
991     nMaxDatacarrierBytes = GetArg("-datacarriersize", nMaxDatacarrierBytes);
992 
993-    fAlerts = GetBoolArg("-alerts", DEFAULT_ALERTS);
994+    RegisterArg(fAlerts, "alerts", DEFAULT_ALERTS);

MarcoFalke commented at 4:04 pm on January 19, 2016:

Could you keep the hypen, so it is possible to grep for it?

Nit: Also, could you make it the first arg so it is easier to fix https://github.com/bitcoin/bitcoin/blob/668906fcf27bdf43a82fe331a377aef55e4b593a/contrib/devtools/check-doc.py#L19

chris-belcher commented at 2:15 pm on March 30, 2016: contributor

Having a getter function too would be helpful.

In JoinMarket sometimes people misconfigure -walletnotify, the getter options would be a way to detect this and display a helpful error message.
dcousens commented at 1:10 am on March 31, 2016: contributor

This sounds like it could cause havoc in some places, especially where the assumption of constants has been made.

Otherwise, concept ACK.
rebroad commented at 1:36 am on September 25, 2016: contributor

concept ACK - will the changed values be remembered so that if bitcoind is restarted it will continue with the last-used values?
luke-jr commented at 3:56 am on September 25, 2016: member

No, for that we’d need to combine this with #7510.
rebroad commented at 4:29 am on September 25, 2016: contributor

@luke-jr ah, in that case I’m strongly in favor of #7510 also then - we surely need a way to enable configuration changes to persist between runs, don’t we?
paveljanik commented at 7:51 pm on December 10, 2016: contributor

As #7510 is too controversial, let’s continue with this. Needs rebase.

Dumping memory pool slightly shifted the priority here, because one can easily restart the node, but still looses connections etc. So still concept ACK from me.
da2ce7 commented at 8:12 am on February 20, 2017: none

@luke-jr, this pull request has had many Concept ACK. I think that it is worth the rebase, and a second look.
luke-jr commented at 7:15 pm on April 20, 2017: member

Not a priority for me right now.
luke-jr closed this on Apr 20, 2017
DrahtBot locked this on Sep 8, 2021

Contributors
luke-jr MarcoFalke jgarzik jonasschnelli paveljanik GIJensen chris-belcher dcousens rebroad da2ce7

Labels
Refactoring RPC/REST/ZMQ