payjoin-cli resume should make max feerate sticky per session

[nothingmuch]

self.config.max_fee_rate is passed to `finalize_proposal.

if you create two receive sessions with different max fee rates, i think the expected behavior should be to remember the value for each session separately when resuming

unfortunately when resuming, the current behavior is that the value given by the -f global option or the config file takes precedence over what may have been set previously

[Mshehu5]

This is just an initial thought on the issue would it make sense to persist the max_fee_rate in the database? Based on PR #873, if the receive_session table is being introduced, we could add a column for max_fee_rate. That way, each session can store its specific fee rate, and during a resume, the session would continue using the same rate instead of defaulting to the global -f option or config value.

[nothingmuch]

@arminsabouri do you think this makes more sense as a column or as an event?

ot1h it's not an event, information is not being learned, it's given a priori

otoh one of the design goals of the event system is to unburden application devs from having to deal with things like DB migration, so having "just a max fee rate column" seems like the start of a slippery slope

[arminsabouri]

Are we expecting max fee rate to change over time? For example, lets say the mempool is congested and Im willing to pay a high fee rate to be competitive. My payjoin fails transiently. If the mempool has cleared up on my next attempt would I want to change that value? In this case a global -f flag does make sense.

do you think this makes more sense as a column or as an event?

Now that we have seperated fee application (#870) from signing, the fee prefrences are being pesisted in the form of a PSBT. Is that sufficient?

[nothingmuch]

Are we expecting max fee rate to change over time? For example, lets say the mempool is congested and Im willing to pay a high fee rate to be competitive. My payjoin fails transiently. If the mempool has cleared up on my next attempt would I want to change that value? In this case a global -f flag does make sense.

Yeah that's another consideration, passing -f to the receive command to make another attempt is important. However, right now the value given at initiation time is completely irrelevant when resuming if there's a config file i think that's just buggy since if any value is given explicitly to the receive command it's it would just be discarded silently, and the fact that resume accepts a value is kinda inconsistent, and will override each active session's initial value depending on their current state.

i guess for payjoin-cli the main priority should be to make it clear to wallet devs how they should deal with this kind of information and their users' preferences, regardless of the way they design the interface for this? in which case maybe a resume command that resumes all sessions is kind of hard to make friendly, usable and fully configurable, but maybe that's OK since it's not its main goal?

Now that we have seperated fee application (#870) from signing, the fee prefrences are being pesisted in the form of a PSBT. Is that sufficient?

seems like it isn't because it's not early enough in the state machine, but thinking about it more i think this implies constraints on the right fee should be handled in the state machine

[Johnosezele]

I think a clean fix is to record that value inside the event log itself.
Since the state machine already replays events to restore its exact history, persisting the fee cap there keeps the flow deterministic.
Relying only on the PSBT isn't sufficient, because the PSBT appears after apply_fee_range. If we crash/resume before that point, we still need the original max fee.

Minimal change:

enum ReceiverSessionEvent {
    /* existing variants… */
    MaxFeeRateSet(FeeRate),
}

1. Right after the CLI parses -f (or the config) and creates the session, then append ReceiverSessionEvent::MaxFeeRateSet(max_fee_rate) to the log.

2. Add a helper on SessionHistory, something like stored_max_fee_rate() -> Option<FeeRate> returning the last such event.

3. In apply_fee_range do:

    let fee = session_history
        .stored_max_fee_rate()
        .unwrap_or_else(|| self.config.max_fee_rate);
    proposal.apply_fee_range(None, fee)

4. If a user intentionally wants to override the cap on a resume they can still pass -f NEW, and we simply write another MaxFeeRateSet event, last one wins.

Since existing sessions don't have this event, the helper returns None and behavior remains as we have.

Happy to put together a PR if this approach sounds reasonable.

[arminsabouri]

@Johnosezele I think this is mostly correct. However, I wouldnt add a new session event for this. Instead make a optional field on the receiver's session context. And in apply_fee_range you can do self.session_context.max_fee_rate.

If a user intentionally wants to override the cap on a resume they can still pass -f NEW, and we simply write another MaxFeeRateSet event, last one wins.

This sounds like it could get messy. If we do want the override functionality I would have apply_fee_range take a max_fee_rate param like it does today and have that value from the -f flag.

[DanGould]

I think our current struct that uses Option on SessionContext for non-optional fields is an anti-pattern that needs to be fixed by splitting the struct into those with precise and non-optional structs. For example, e: Option<HpkePublicKey>, doesn't make much sense. A v1 receiver has no e, and a v2 receiver must have an e for example, so these two things deserve separate states.

I want to be clear here that max_fee_rate becomes an an Option<FeeRate> on SessionContext if and only if it is truly optional. Is it truly optional?

[Johnosezele]

@Johnosezele I think this is mostly correct. However, I wouldnt add a new session event for this. Instead make a optional field on the receiver's session context. And in apply_fee_range you can do self.session_context.max_fee_rate.

If a user intentionally wants to override the cap on a resume they can still pass -f NEW, and we simply write another MaxFeeRateSet event, last one wins.

This sounds like it could get messy. If we do want the override functionality I would have apply_fee_range take a max_fee_rate param like it does today and have that value from the -f flag.

Thanks @arminsabouri , I'll drop the extra event and instead add a max_fee_rate field to the receiver's session context.
On resume we'll use that stored value unless the user explicitly passes -f, in which case the CLI will overwrite the field before re-entering apply_fee_range().

[Johnosezele]

I think our current struct that uses Option on SessionContext for non-optional fields is an anti-pattern that needs to be fixed by splitting the struct into those with precise and non-optional structs. For example, e: Option<HpkePublicKey>, doesn't make much sense. A v1 receiver has no e, and a v2 receiver must have an e for example, so these two things deserve separate states.

I want to be clear here that max_fee_rate becomes an an Option<FeeRate> on SessionContext if and only if it is truly optional. Is it truly optional?

I think for the purpose of this fee-rate bug, I'd like to keep changes minimal and just add max_fee_rate in the context we already have (non-optional, because a v2 receiver must always have one).

Agreed, this current struct with Options is an anti-pattern.

The broader cleanup you're proposing (splitting SessionContext into precise per-state structs and removing the bogus Options) is bigger than this bug, so I'll open a follow-up issue and reference your comment so we can tackle it separately.