spec: maxfeerate, revoke_order, preimage response ack, feedless orderbook by buck54321 · Pull Request #526 · decred/dcrdex

buck54321 · 2020-07-03T17:13:10Z

config response "feerate" field changed to "maxfeerate". Changes implemented in multi: match fee rates and smart fees #505. Resolves spec: messaging update for fee rate changes #521
A new revoke_order message to be sent to clients when their orders are revoked.
A response acknowledgement for the preimage request. Possible resolution of comms: handle response errors #456.
A new "nofeed" field in the orderbook request, which disables subscription to the order book feed. Can be used in client/rpcserver: Add orderbook route. #496.

chappjc · 2020-07-03T17:15:26Z

resolving the comments at https://github.com/decred/dcrdex/pull/525/files#diff-5c4d3ee4527d6c50d1379b8edb5ef11eR2502-R2504 ("match revocations are sent to both parties")? Would that be used when the market is flushed (persist=false) also, or would the suspend message be considered adequate there?

buck54321 · 2020-07-03T17:20:38Z

Yep, though persist = false is already handled in #525.

chappjc · 2020-07-07T20:02:00Z

+| swapconf   || int || minimum confirmations for swap transactions
 |}



After this object definition, can we link to Match Negotiation in orders.mediawiki, perhaps also saying that the maxfeerate is applied to the swap contract transaction as described in that section, as "assigned to matches" is a little vague.

chappjc · 2020-07-07T20:06:00Z

+|-
+| ackid   || int || the message ID to be used for the server's preimage acknowledgement
 |}

-==Match negotiation==
+The server will send an [[comm.mediawiki/#Acknowledgements|acknowledgement]] of
+their receipt of the preimage. The message ID of the acknowledgement will be set
+to the specified <code>ackid</code>. The signature message is the byte-encoded
+preimage.


It seems to me that if the server wanted to exclude an order from the shuffle, they would just not ack, so it's not clear if this adds a lot unless we adapt it to communicate error conditions with the response handling.

unless we adapt it to communicate error conditions with the response handling

That is the purpose here.

chappjc · 2020-07-07T20:14:54Z

 ! field   !! size (bytes)  !! description
 |-
 | pimg    || string || hex-encoded preimage for the order's commitment
+|-
+| ackid   || int || the message ID to be used for the server's preimage acknowledgement
 |}


I understand this is part of a solution to the need for a response to a response, but I'm not sure if/how it will work out allowing the client to specify the message id for an outgoing server message. Although I think only requests really need to use comms.NextID to ensure a unique ID while it does not matter for notifications and responses. Pretty sure this is not a problem with that?

An alternative would be do change the acknowledgement payload to contain an ackid also so that the containing message's ID would continue to be serial, monotonically increasing.

Also, part of the need for a response to a response was in the case of an error during response handling requiring the server to inform the client of the error in processing their response, so could we use this ackid concept for errors? Maybe new fields in the acknowledgement payload for success and general-purpose text details along with the signature?

What do you think about flipping the semantics a little bit, and instead of sending a response to a response, we can instead send request for a request? For instance, the DEX-originating preimage request triggers the client to submit a submit_preimage request that just references either the preimage request ID, or maybe even just the market and order ID. It would require some reworking on the server side, since we can't use a request callback to process the preimage data. Just a thought.

I think either request to a request, or a response to a response could be made to work. I've been mulling over the protocol recently I was imagining a request or response with no id set to indicate that it's not a usual request/response and there's a reference id in the payload instead, and it should be treated as both a request (requires a response) and a response (there should be a registered response handler). Alt. the id could be set in the usual place, but a new msgjson.Dialog type that signals the dual role.

Although, defining a new request like submit_preimage would be perhaps cleaner if not as generalized to the myriad of cases where we need this kind of communication sequence such as the match ack response from the client needing its own response so the client knows when it's OK to init (https://github.com/decred/dcrdex/pull/565/files#r460186427).

I guess one of us just needs to try out some ideas and start hacking.

Alt. the id could be set in the usual place, but a new msgjson.Dialog type that signals the dual role.

I like this variation. The payload can then contain something like a sub-route as a sequencing field.

chappjc · 2020-07-07T20:17:40Z

+The client will respond with an
+[[comm.mediawiki/#Acknowledgements|acknowledgement]].


I'm also not sure if this ack serves a real purpose either. What would the server do with the client's ack? Much like with revoke_match, the server does nothing with the ack presently, just logging it.

You okay with removing both acknowledgements and making these notifications instead?

buck54321 · 2020-08-13T19:52:20Z

Going back to draft while I make some changes.

buck54321 · 2020-08-19T19:21:53Z

I've roughed out a dialogue-type message and used it for the preimage. Leaving in draft, but critique is welcome.

chappjc · 2020-08-20T14:52:36Z

+	"id": 100,
+	"returnid": 200,
+	"route" "exchangeinfo",
+	"payload": {"info"; "recipient_data_1"},


Would all subsequent messages/responses not be using a ResponsePayload as the Message.Payload? This shows a free-form payload structure with no "response" or "error" keys. This could work if "error" is a possible key that the handler for this route would look for to detect an early dialog termination. More on this in my subsequent comments...

I wanted to leave it as general as possible, but maybe you're right that we should use ResponsePayload or similar because errors.

chappjc · 2020-08-20T14:56:48Z

+'''Dialogue step 2: preimage reveal''', '''originator:''' client

 <code>result</code>
 {|
 ! field   !! size (bytes)  !! description
 |-
 | pimg    || string || hex-encoded preimage for the order's commitment
-|-
-| ackid   || int || the message ID to be used for the server's preimage acknowledgement
 |}


This step is headed with <code>result</code>, suggesting that this data (a PreimageResponse) would be encoded into the "result" of a ResponsePayload, which goes inside a Message.Payload, as it is presently. I got the impression the Message.Payload would be whatever, but a ResponsePayload would work too.

Sorry. That should say payload.

chappjc · 2020-08-20T15:01:59Z

+the original message ID. The <code>returnid</code> is only expected to be set in
+the first non-initiator message. The initiator will continue to use that ID for
+all subsequent messaging.


For my comment below, s/non-initiator/participant/, but we could do that in the spec too if you like.

My understanding of these lines is reflected by slightly different language, but I think we are saying the same thing:

A "returnid" field is only present in the participant's dialog initialization response. In all subsequent messages, the initiator will use the value of that field for the "id" field in dialog messages to the participant, while the participant will continue to use the initiator's "id" from the initialization request.

Spelled out with some implementation hints, what I think we are describing is this:

The dialog initialization request from the initiator would have "id": x. Before sending the initialization request, the initiator registers some dialog-scoped data or a handler function for the dialog keyed by x.

The participant gets the request and uses "route" (and "type": 4) to find the associated global handler to start this dialog.

The dialog initialization response from the participant has "id": x and "returnid": y. Before sending this message, the participant registers some dialog-scoped data or a handler keyed by y for further messages for this dialog, storing the value of x with this data/handler for dialog y.

Initiator gets the initialization response with "id": x, finds the data/handler for they dialog keyed by x, and stores the value of y just provided in "returnid".

Now both parties have some dialog-scoped data or a handler in a map keyed by their own unique id that stores the other party's unique id. The dialog is established.

Subsequent messages from the initiator have "id": y so that the participant can find the dialog.

Subsequent messages from the participant have "id": x so that the initiator can find the dialog.

The relevance of the "route" field in all dialog messages depends on the implementation. Options alluded to above:

a request-response approach where the initial message from each party creates a new handler for the dialog, not needing the route except for the handling of the dialog initialization request by the participant's global request handler

an "all requests" approach where the global request handler for the specified route is always used, but this global handler attempts to locate dialog-scoped data from some map specific to this route.

I actually like the second option, but hacking on this might change my mind.

Your implementation description is spot on, and the "all requests" approach is the one I was picturing, but like you said, let the hacking flesh it out. No need to rush this out of draft either. Might be time to just do it, and implement the solution for preimage errors (#597), and then come back to this.

May also be a good candidate for the match request, so that the client can get confirmation that their ack was recorded before starting their 'init' request. ref: #565 (comment) Perhaps I'm seeing a logic race where there isn't one, but it seems like an OK is needed back from the server before the maker does its 'init' request, otherwise the server could be not ready.

But there are a host of server-originating requests that may be best off as dialogs to allow the server to respond with an error to the client's response.

chappjc · 2020-08-20T15:35:52Z

+the original message ID. The <code>returnid</code> is only expected to be set in
+the first non-initiator message. The initiator will continue to use that ID for
+all subsequent messaging.


Thinking about termination of the dialog, there would presumably just be a known dialog sequence for this particular route, which could terminate early with an error or run to completion.

Yep. The specification for the route will define any required sub-sequencing requirements, but really it'll probably just be that both sides remember what step they're at.

chappjc · 2022-02-24T17:03:10Z

The whole spec is getting a review and update as part of phase 3, so I suggest we start fresh. I'm not sure how accurate this current change is anymore given how much has changed, but if it's basically right we can finish this one off. But would be nice to close up some of the older PR drafts.

chappjc · 2022-06-09T03:30:27Z

See pinned issue #1641
Need a fresh start

chappjc requested changes Jul 7, 2020

View reviewed changes

chappjc added the spec label Jul 14, 2020

buck54321 marked this pull request as draft August 13, 2020 19:51

buck54321 added 4 commits August 19, 2020 13:01

maxfeerate, revoke_order, preimage response ack, feedless orderbook

d24bbc3

generalize acks language. link to match negotiation in maxfeerate desc.

15baa3c

add unmatched orders header

55ce6f2

add dialogue-type message

8ed901f

buck54321 force-pushed the specstuff branch from b642c0a to 8ed901f Compare August 19, 2020 19:20

chappjc reviewed Aug 20, 2020

View reviewed changes

itswisdomagain mentioned this pull request Sep 8, 2020

client/core,asset: ensure coins are unlocked when no longer needed #648

Merged

chappjc mentioned this pull request Sep 14, 2020

new match ack dialog should also prevent maker swap until BOTH acks #686

Closed

chappjc closed this Jun 9, 2022

		\| swapconf \|\| int \|\| minimum confirmations for swap transactions
		\|}

		The client will respond with an
		[[comm.mediawiki/#Acknowledgements\|acknowledgement]].

Conversation

buck54321 commented Jul 3, 2020 • edited by chappjc Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chappjc commented Jul 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

buck54321 commented Jul 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chappjc Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

buck54321 Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

buck54321 commented Aug 13, 2020

Uh oh!

buck54321 commented Aug 19, 2020

Uh oh!

chappjc Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chappjc Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chappjc Aug 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chappjc commented Feb 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chappjc commented Jun 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

buck54321 commented Jul 3, 2020 •

edited by chappjc

Loading

chappjc commented Jul 3, 2020 •

edited

Loading

buck54321 commented Jul 3, 2020 •

edited

Loading

chappjc Jul 28, 2020 •

edited

Loading

buck54321 Jul 29, 2020 •

edited

Loading

chappjc Aug 20, 2020 •

edited

Loading

chappjc Aug 20, 2020 •

edited

Loading

chappjc Aug 25, 2020 •

edited

Loading

chappjc commented Feb 24, 2022 •

edited

Loading