units: add consolidate_feerate as cli arg - #77

I added a commit to match more closely the conventions used in the core client (see commit message).

For latent safety purposes would it make more sense to put the 10 sat/vB default at the ipc.rs boundary, with a named constant?

// mirrors bitcoin-core's DEFAULT_CONSOLIDATE_FEERATE (10 sat/vB)
const DEFAULT_CONSOLIDATE_FEE_RATE_SAT_PER_VB: f64 = 10.0;

let raw = p.get_consolidate_fee_rate_sat_per_vb();
if !raw.is_finite() || raw < 0.0 {
    return Err(capnp::Error::failed(
        "consolidate fee rate must be a non-negative number".to_string(),
    ));
}
let long_term_sat_per_vb = if raw == 0.0 {
    DEFAULT_CONSOLIDATE_FEE_RATE_SAT_PER_VB
} else {
    raw
};

For latent safety purposes would it make more sense to put the 10 sat/vB default at the ipc.rs boundary, with a named constant?

Thanks for the suggestion, although, im not sure I fully understand. Clap allows default arguments, so in the CLI options, there needs to be something done if the option is not provided I believe. Are you suggesting changing the way the defaults are handled in cli.rs as well as the suggestion above?

I think the other part of your suggestion is that if a value is provided, and the value is 0.0, that it should be changed to a default value instead? Not sure, I find that not so intuitive and maybe we should just error on a 0.0 consolidate_fee_rate. However, it looks like currently 0.0 for fee-rate is accepted, which I think is actually a problem since the network will never propagate a transaction with a 0.0 feerate.

Right now 0 is accepted. My preference is to let the server own the default, which looks like how Core does it. Erroring on 0 makes sense but pulls the default back onto the CLI and capnp can't tell "sent 0" from "sent nothing," so once you reject 0 the client has to supply the value. That's what I was trying to avoid, so I lean server-side default.

Right now 0 is accepted. My preference is to let the server own the default, which looks like how Core does it. Erroring on 0 makes sense but pulls the default back onto the CLI and capnp can't tell "sent 0" from "sent nothing," so once you reject 0 the client has to supply the value. That's what I was trying to avoid, so I lean server-side default.

It sounds like a good idea to enforce the default on the server. However, I'm still a little unclear how the CLI should behave when it gets no argument, since it must then set a value which may as well be the default. Never the less, I have added your suggestion as a fixup even though this now means the default is being set twice. Please let me know if this is what you had in mind or what I am missing.

If there's no further feedback I can autosquash the commits then. I don't think Github is smart enough to autosqush on merge.