Persistable payjoin types

[arminsabouri]

Drafting this for now. This is at feature parity with the current functionality of payjoin-cli. Persistance is now implemented for both the sender and reciever. We are only persisting when the sender and reciver are first intialized. The trait design is architected in such a way that its easily expandable for other state of the senders and reciever. For example if a application is expecting to save the state at init and finalize and non of the intermediate steps that is an option. And for the reciever I would advocate in a seperate PR that we persist when the reciever when first crated, proposal fetched and payjoin created.

There is alot to bikeshed when we consider persisting other reciever & sender states. We'll cross that bridge that when we get there but I will say this design is more friendly towards persisting other states, compared to #560.

Alternative design of the same functionality: #560
Issue: #336 , #477

[nothingmuch]

I would defiently welcome some feedback on the format type. Should we offer different options? i.e as_json() and as_bytes()?

I think that should be up to the implementing wallet, this persistable trait is kind of an inversion of my suggestion which was to instead have the payjoin crate types accept a trait argument that is the thing they use to serialize themselves, i.e. something like:

trait Persister {
    fn save<T: Serialize>(&self, key: Key, value: T) -> Result<(), PersistableError>;
    ...
}

then it would be the responsibility of the state machine methods to use this abstraction to call save etc at the appropriate time. implementation of the trait would just see a black box KVP.

the Database struct in the payjoin-cli crate can then be refactored to use this lower level Persister trait (with simple filtering to separate the sessions by type), and a sled based impl of the Persister trait would also be available.

it's not clear to me what to do with regards to pruning storage, i guess the simplest thing is to also have the ability to enumerate all of the keys (which resume would need anyway) and then just filter out those that expired, so that the trait would just need a way to iterate keys, and save, load, and delete methods?

[nothingmuch]

another thing I'm not sure about is how to keep track of IO related state. for example, when the receiver constructs their response, the state machine logic can serialize that, and then up to the integrating wallet to actually deliver that to the directory. if there is a transient error, retrying makes sense, so keeping track of a successful broadcast in the state machine might also make sense?

perhaps a better abstraction, and one that does not rely on traits, is to have the return types from the state machine methods indicate what actions the integrator needs to do, e.g. you get returned a data container that first you must serialize with, that unlocks the actual request data that you need to send, and finally you need to confirm that this happened successfully before you you can load it to resume? that would be a bit more verbose, but removes all side effects including indirect ones

[coveralls]

Pull Request Test Coverage Report for Build 14149717767

Details

• 113 of 133 (84.96%) changed or added relevant lines in 7 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage increased (+0.008%) to 81.707%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/receive/v2/mod.rs	19	20	95.0%
payjoin/src/send/v2/mod.rs	27	28	96.43%
payjoin-cli/src/db/error.rs	0	2	0.0%
payjoin/src/send/multiparty/mod.rs	21	24	87.5%
payjoin/src/persist/mod.rs	2	15	13.33%

Files with Coverage Reduction	New Missed Lines	%
payjoin/src/directory.rs	1	96.0%

Totals
Change from base Build 14136225234:	0.008%
Covered Lines:	5190
Relevant Lines:	6352

---

💛 - Coveralls

[arminsabouri]

then it would be the responsibility of the state machine methods to use this abstraction to call save etc at the appropriate time. implementation of the trait would just see a black box KVP.

Im a fan of this however there are some trade offs.

1. This would break the v1::Sender/Reciver interfaces at some level - maybe thats fine. For example, when we are building the sender we would have to pass in a impl Persister. I'll provide a noopPersister and if the applications wants to opt out then can just intialized a sender with None and I will unrwap to the noopPersister.
2. What does this mean for the bindings? How does the persister trait get represented in something like JS?
3. Some applications are going to do their persistance I/O in an async runtime. Its not uncommon. Supporting an async trait would blow up the complexity of this PR. We could just say "we dont support this, find a workaround".

Shouldn't be too hard to PoC this.

[nothingmuch]

1. This would break the v1::Sender/Reciver interfaces at some level - maybe thats fine. For example, when we are building the sender we would have to pass in a impl Persister. I'll provide a noopPersister and if the applications wants to opt out then can just intialized a sender with None and I will unrwap to the noopPersister.

I don't have a better suggestion than that

2. What does this mean for the bindings? How does the persister trait get represented in something like JS?

I think rust glue to make a trait impl that delegates to callbacks would suffice

3. Some applications are going to do their persistance I/O in an async runtime. Its not uncommon. Supporting an async trait would blow up the complexity of this PR. We could just say "we dont support this, find a workaround".

One option is to have a blocking impl wrap around calls to an async runtime.

Another is to support an async trait, instead.

A third is to retain non-blocking, purely functional behavior from the main types, but implement my alternative idea, i.e. return an struct that wraps around the current return values, and adds the data to persist, and forces the caller to first serialize the data to persist (or at least pretend to do that) before giving back e.g. the extracted request or whatever it is that is returned using the current return types

[arminsabouri]

This would break the v1::Sender/Reciver interfaces at some level -

I was able to reduce the footprint quite a bit by passing in impl Persister on the state transition functions. For example

pub fn check_broadcast_suitability<P: Persister>(
        self,
        min_fee_rate: Option<FeeRate>,
        can_broadcast: impl Fn(&bitcoin::Transaction) -> Result<bool, ImplementationError>,
        persistor: P,
    ) -> Result<MaybeInputsOwned, ReplyableError>
    where
        P::Key: From<bitcoin::Txid>,
    {
        let inner = self.v1.check_broadcast_suitability(min_fee_rate, can_broadcast, persistor)?;
        Ok(MaybeInputsOwned { v1: inner, context: self.context })
    }

It would make sense to just have the persister top level in the Reciver or Sender but these structs are often serialized directly to DB. At least payjoin cli does this to resume the PJ session, not sure if the other wallet do the same.

This resume functionality could be replaced all together using the new persister paradigm. e.g you can persist the state machine and pick up where you left off for each individual payjoin session you have.

[nothingmuch]

I was able to reduce the footprint quite a bit by passing in impl Persister on the state transition functions. For example

I like how this is looking

nit: spelling varies, persistEr vs. persistOr

It would make sense to just have the persister top level in the Reciver or Sender but these structs are often serialized directly to DB.

Hmm, wouldn't initializing such a field after deserialization kinda reduce to the same approach as passing it into various state transition functions as in your example? Perhaps a session manager struct that has that and which reduces the boilerplate is in order, but I think that's a bit premature, I think for now we should figure out the low level stuff and make sure it removes the burden from understanding when is the right time to serialize from implmentors, and then see how we can reduce the boilerplate.

With that in mind, perhaps the persister API should return a kind of storage token for lack of a better term (conceptually like a ticket at a bag check), like Ok(Foo(...)) where for noop Foo is just a newtype wrapping whatever was passed in, but for real persisters it's just the key, so passing that back to the persister gives back the same session. Then the state transition methods could consume the Receiver and Sender, ensuring state can't roll back enforced by the borrow checker. I'm not sure if that's too restrictive, but doing it that way could ensure that sessions can't accidentally be cloned/forked, assuming compliant persister impls load doesn't just return a copy but uses some kind of mutex/lock mechanism (and in the noop persister, just transfer ownership but, restrict access, i.e. Foo.0 is not pub but requires passing it back to the persister to "load" it back).

This resume functionality could be replaced all together using the new persister paradigm. e.g you can persist the state machine and pick up where you left off for each individual payjoin session you have.

Do you mean as a CLI command? If not then isn't that already what's happening under the hood?

[arminsabouri]

Do you mean as a CLI command? If not then isn't that already what's happening under the hood?

yes the CLI command. My understanding of resume is that it always starts the type state machines from their initial states. For example, if the reciver made it all the way to ProvisionalProposal, resume would just restart from UncheckedProposal. If you persist as every step via Persister, its possible to resume at any state.

With that in mind, perhaps the persister API should return a kind of storage token for lack of a better term (conceptually like a ticket at a bag check)

Im not sure im fully grasping this idea. The problem being solves seems to be around preventing accidental forks and multiple parallel sessions (which im not sure what the worst case senario there is. Is it possible you can have two parallel PJs spending different UTXOs? That seems like its an application responsibility).
But I think im on the same page b/c this seems to address a different problem i have having which is how does the app know what struct they are deserializing. I was thinking we could also store an enum. In the reciver case you could have 33 byte ids. First 32 Bytes are the original pj txid last byte encode which state you are in. Could bump up to u16 incase we have to additional state machines, for instance in the multiparty case.

[nothingmuch]

Im not sure im fully grasping this idea. The problem being solves seems to be around preventing accidental forks and multiple parallel sessions (which im not sure what the worst case senario there is. Is it possible you can have two parallel PJs spending different UTXOs? That seems like its an application responsibility).

Specifically I'm thinking of something like the receiver responding twice with two different payjoin PSBTs to the same original PSBT due to a loss of state. The damage in this case would be leaking more UTXOs than intended to the sender.

If persistence and processing is done in lockstep, then we can help the application ensure that the response is only processed once, whereas if it's the application's responsibility to persist state information at the right time and with the right semantics, a bug causing processing to happen twice is more likely, especially if treating payjoin stuff as a black box and not studying the protocol in detail.

I was thinking we could also store an enum. In the reciver case you could have 33 byte ids. First 32 Bytes are the original pj txid last byte encode which state you are in. Could bump up to u16 incase we have to additional state machines, for instance in the multiparty case.

Shouldn't the state be part of the serialized value, not the key?

FWIW serde supports tagging enums with the type name, that seems like less hassle (esp. wrt database format upgrades) than assigning ints, but that's also supported.

While we have struct types representing state, they are not quite the typestate pattern since there's no single parameterized type wrapping these, and there also isn't an enum representing the tagged union of possible states as a single type, i suspect there might be an idiomatic approach to this that also takes care of serialization,

[arminsabouri]

i suspect there might be an idiomatic approach to this that also takes care of serialization,

We could wrap the actual typestate struct in a top level enum. There we can take advantage of the tagged enum.

#[derive(Debug, Clone, Serialize, Deserialize)]
#[serde(tag = "state", content = "data")]
pub enum PayjoinProposalState {
    Unchecked(UncheckedProposalData),
    MaybeInputsOwned(MaybeInputsOwnedData),
    MaybeInputsSeen(MaybeInputsSeenData),
    OutputsUnknown(OutputsUnknownData),
    WantsOutputs(WantsOutputsData),
    WantsInputs(WantsInputsData),
    Provisional(ProvisionalProposalData),
    Final(PayjoinProposalData),
}

Using serde_json for example would give us a output of

{
  "state": "Unchecked",
  "data": {
    "psbt": "...",
    "params": "..."
  }
}

I can get a POC on this pretty quick

[arminsabouri]

Shouldn't the state be part of the serialized value, not the key?

Depends. If we want to store each indiviual state then it would make sense if the state type was encoded as the key. Otherwise in KVP DBs apps would just overwrite db entries

[nothingmuch]

Depends. If we want to store each indiviual state then it would make sense if the state type was encoded as the key. Otherwise in KVP DBs apps would just overwrite db entries

If they aren't overwritten then doesn't that mean it's trivial to accidentally restore a stale state and resume the state machine from that state?

[arminsabouri]

Depends. If we want to store each indiviual state then it would make sense if the state type was encoded as the key. Otherwise in KVP DBs apps would just overwrite db entries

If they aren't overwritten then doesn't that mean it's trivial to accidentally restore a stale state and resume the state machine from that state?

No b/c you could sort by the last byte which is the index into state machine; Overwriting would make life simpler for the developers but my impression of the ticket discription is that we want to persist every state transition.

[nothingmuch]

No b/c you could sort by the last byte which is the index into state machine;

In a KVS wouldn't that be better expressed by storing a list of states underneath a single key instead of needing to scan all the keys and filter by prefix to find the ones associated with a particular session, then sort them?

Overwriting would make life simpler for the developers but my impression of the ticket discription is that we want to persist
every state transition.

As long as information is not lost by overwrites, namely the original PSBT & bip21 URI, which can be useful for interpreting the final txn, I don't think all states need to be strictly stored.

Also, for looking up this historical data, either the ID should eventually become the final txid for both, or we'd want to have some sort of auxiliary index to avoid needing to scan all old sessions in order to find historical information associated with a confirmed txn? In a flat KVS, this can be done in the same key space by just storing an entry with the final txid with the internal ID that was used prior to learning that final txid as the value.

Either way I'd be in favor of packaging all of a session's data under a single KVS key, serializing the history of states should be negligibly costlier than just the new information, it ensures everything is sorted so there's never any ambiguity about the latest state, esp. in concurrent setting where multiple disparate keys can't be updated atomically with most KVSs (but individual ones usually can).

[arminsabouri]

In a KVS wouldn't that be better expressed by storing a list of states underneath a single key instead of needing to scan all the keys and filter by prefix to find the ones associated with a particular session, then sort them?

yeah lets go with this. Also the application can decide how and if they want to store every state. Save() will be called either way.

[arminsabouri]

A couple noteable changes:

• I split out the persisters to a v1 and v2. v2 will use the session ShortID as the key and v1 will continue to use the the original psbt txid. V2 will persist the v2 type state which includes the session. And will pass down a noop persister to v1 so we don't double persist.
• I also added the persister to the reciever::new(...)
• I refactored resume functionality in payjoin-cli to use the persister paradigm. Currently we are persisting two reciever states. Uninitlaized which is the reciver never got a payload from the directory and Unchecked which is the first type state after we reciver a proposal from the sender.
  This refactored is backwards compatible with how resume works today and it has the added benifit of skipping the long-poll step if we have already done it. Note that this states will override each other as they use the same shortID. The resume function has no need for a full history of the state transitions but if an app did have this requirment they could simply store all the state transitions under the same key.

It may be prudent to also persist after we have contributed inputs. Since inputs contribution will not be deterministic, this could lead UTXO probing. But at the same time, those inputs could have been spent. I'll leave this as open work for the future as its outside the scope of the ticket.

Open question: does it make sense to impl persister for any other type states? @DanGould had mentioned in a irl convo that it could make sense to save the last state PayjoinProposal. It would be fairly trivial to resume from the last state but we do hit the same issue as above where does inputs may have been spent before resume was called. Maybe thats an application concern, not a library one.

EDIT: I couldn't justify having a v1 persister. Simply wanst getting used anywhere. If there is demand for it I would be trivial for me to add it back

[DanGould]

One option is to have a blocking impl wrap around calls to an async runtime. — @nothingmuch

FWIW this is essentially what we do with state machine closures since there is no stable, first-class syntax for “async closures”. A simple save closure could just pass |key, value| that are both Serializable and return a Result<bool> whether closure saved or not. I don't think you'd need load functions in this design since you'd just be deserializing values from saved keys. I think that would be the simplest design to enforce persistence without traits or generics or enums.

While we have struct types representing state, they are not quite the typestate pattern since there's no single parameterized type wrapping these, and there also isn't an enum representing the tagged union of possible states as a single type, i suspect there might be an idiomatic approach to this that also takes care of serialization,

Indeed, we're using structs, and maybe "state machine" has more semantic correctness than typestate machine to describe our implementation.

it could make sense to save the last state PayjoinProposal

The downstream wallet should save anything it signs or wishes to spend. Participants should save orignal Psbt info in order to recall fees it spent outright vs fees spent by the sender to incentivize payjoin and to show the upgrade path to a user, or to broadcast it in case payjoin fails. Receivers should save payjoin PSBTs so that they know what upgraded PSBT failed, for feerate reasons or otherwise.

Enumerating Points of Persistence in payjoin-cli reference

There are only four places where persistence occur in our send/receive flow in order to enable async functionality (i.e. resuming) in the current payjoin-cli reference. The other payjoin-cli/src/db/v2.rs functions not enumerated here are used to fetch state from memory for resumption.

1. insert_recv_session

Called immediately upon Receiver::new(..)

Could add a closure or have a state transition EphemeralReceiver::persist() -> SavedReceiver where only SavedReceiver defines extract_req.

2. clear_recv_session

Called as soon as PayjoinProposal::process_res returns successfully, since the response must have been posted, and the receiver should not to respond to an original psbt with two different messages to the directory. The wallet should have taken over at this point since it saved the Proposal PSBT it signed.

3. insert_send_session

Called as soon as SenderBuilder::build() returns a Sender, because by then an Original PSBT has been produced to satisfy a given bip21.

The implementation here smells to me in that resumed senders will re-POST the message A and process a response before they poll using the V2GetContext. Perhaps it would make more sense to either discard the Sender if there is no response after that initial POST and only save the V2GetContext to be resumed, OR to persist both. It does make some sense to attempt Message A again if there's a network issue, but it doesn't make sense to re-POST after a success response.

If were the author of this PR I'd just solve for the lack of persistence bit within the existing flow first enforced by a state machine, and then concern myself with the question of whether or not persistence is done at the right times in the machine or not.

4. clear_send_session

Called as soon as process_pj_response returns Ok after polling V2GetContext, checking, signing, and broadcasting a successful payjoin. At this point the sender should not re-send the original_psbt since at this point the sender would have broadcast a Payjoin.

It seems to suffer from a similar issue to insert_send_session, that, there may be other cases to clear, or better yet mark the send session as failed, outside of the current place it's called. Still, I'd just come up with a design to enforce a persistence step first and only then worry about placing the persistence correctly in a follow up commit in the same PR.

---

Perhaps the existing persistence flow is inadequate, but either way I would pick a design to satisfy the existing flow, either making persistence part of the state machine OR testing and ensuring persistence actions happen everywhere it needs to rather than combining them into one commit.

[arminsabouri]

TODO:

• Remove persistence at check broadcast suitability if in the future we want to be able to persist at these other state transitions we can assume the persiter will be available somehow. But probably shouldnt be passed in at every state transition

[arminsabouri]

Another comment brought up offline w/ @DanGould was to seperate out all persistanc related implementation details to a persist.rs in both receiver/sender

[DanGould]

Even if keys had internal strings, I think we'd want them to be strong types so that save / load are constrainted to their own state machines. Which is here now. The design in general looks good to me. My comments are mostly about keeping our commit log and history tidy and the type of interface for Key that I think we should have to display it in errors.

think this is ok for now esp since there is no refrence impl for ns1r. Altenatively we could pass a noop here with a TODO that this will be implemented once there is a ns1r refrence impl. This is what I originally had.

Since the implementation is straightforward I prefer what you've done here, and actually implemented. I don't see a huge reason to take on tech debt when there are the other implementations to follow.

If debt is prefered, I think intentional exclusion, as seems to have been done with multiparty Receiver, makes more sense than a default NoopPersister.

The commit log references Persistable which is no longer.

I'd also like to see Key impl Display instead of using to_hex_string from the result of AsRef

[DanGould]

Since this duplicates receiver::ImplementationError would be nice to de-duplicate in lib.rs or a core/error.rs module in a follow up.

[arminsabouri]

If we can do this in a follow up i would prefer that

[arminsabouri]

Getting a 405 from codecov. Doenst seem like this is a problem on our side

🔍 Detected coverage file: lcov.info
🚀 Posting coverage data to https://coveralls.io/api/v1/jobs
HTTP error:
---
Error: Method Not Allowed (405)
Message: <html>
<head><title>405 Not Allowed</title></head>
<body>
<center><h1>405 Not Allowed</h1></center>
<hr><center>nginx</center>
</body>
</html>

[DanGould]

As far as I can tell the commit messages still reference an API that is no longer what is implemented:

"commit adds a new Persister parameter to v2::Sender/Receiver::new" I don't think this is done with a new function any longer. May you please fix this and then do a thorough self review before the next iteration?

[DanGould]

Another itch is that persist::Value name is perhaps too generic. I don't care if it's still V in the persister definition, but following my own comment re: understanding of naming, might persist::Persist be more appropriate for types you might want to persist?

[arminsabouri]

Persist

I think Persist::persist is less clear than value. Persist::Value is self-explanatory

[DanGould]

ACK 9e137e3

Hitting deadlines is a skill. 🎆

There's definitely still a lot of funkiness around the existing cli implementation that exist outside of the Persister paradigm. e.g. get_send_session knows the key is a Url to resume send_payjoin, but what in the API suggests that? Should there be a SenderToken::new function to parse it from a bip21?

Same with get_send_sessions and get_recv_sessions, Should there be a Persister function that returns an Iterator?

And finally there is clear, which is meant to mark a session as finished and no longer going to resume. Unclear whether CLI persistence should be fixed to be semantically correct first or whether a Persister implementation should abstract over current behavior, but we probably need a tracking issue to watch progress here in a series of follow ups

[DanGould]

I think this comment is meant to live with Receiver, NewReceiver is the unpersisted variety whose only purpose is to be persisted

[spacebear21]

I think all my comments can be addressed in follow-ups and aren't show-stoppers but I wanted to make note of them before this gets merged.

[spacebear21]

This docstring should be updated since it now creates a NewReceiver

[spacebear21]

Needs docstring

[spacebear21]

Needs docstring

[spacebear21]

Perhaps this should live in receive/v2/persist.rs to separate concerns between persistence stuff and the receiver state machine. (With a corresponding send/v2/persist.rs for sender persistence stuff).

[spacebear21]

nit: these can be chained since the "new_receiver" variable isn't really useful

Suggested change

	let new_receiver = NewReceiver::new(mock_address, directory, bad_ohttp_keys, None)?;
	let storage_token = new_receiver.persist(&mut NoopPersister)?;
	let storage_token = NewReceiver::new(
	address.clone(),
	directory.clone(),
	ohttp_keys.clone(),
	Some(Duration::from_secs(0)),
	)?
	.persist(&mut NoopPersister)?;

[spacebear21]

Having a SenderBuilder return a NewSender that must then be persisted to get a Sender seems somewhat awkward. Usually a "XBuilder" returns a "X". I'm not sure whether it makes sense to merge the SenderBuilder and NewSender steps but this feels slightly off.

[spacebear21]

This seems like a duplicate of the SenderToken in send/v2/mod.rs, could that be imported here?

[spacebear21]

Needs docstring. Also, the SenderBuilder docstrings may need to be updated to reflect that it now returns a NewSender.

[spacebear21]

Needs docstring