v0 Oracle Changes - Githubissues

nkohen commented 3 years ago

As per our spec meeting discussions, this PR:

Splits the oracle key into separate announcement and attestation keys
Adds Schnorr signatures by the nonce keys to the announcement message
Allows for range timestamps in oracle events

nkohen commented 3 years ago

I've added a new oracle_keys message but I'm not sure I'm the best person to write in Oracle.md about how it is used, and likewise to write rationales for why we need two public keys and all the proofs of knowledge. @LLFourn would you be willing to write at least these last two things?

LLFourn commented 2 years ago

Meeting note: attestation schemes should be declared separately from the event information. Taking this as an example:

$ curl 'https://h00.ooo/x/BitMEX/BXBT/2022-01-12T01:00:00.price?n=20' | jq .announcement.oracle_event.data -r  |jq

 {
    "id": "/x/BitMEX/BXBT/2022-01-12T01:00:00.price?n=20",
    "expected-outcome-time": "2022-01-12T01:00:00",
    "descriptor": {
        "type": "digit-decomposition",
        "is_signed": false,
        "n_digits": 20,
        "unit": null
    },
    "schemes": {
        "olivia-v1": {
            "nonces": [
                "ea4c2c81a65e4df1c98515bf2af6a0e63ed086f803e4637130447056f2ff15c8",
                "e7a46ba921fab39fe9e953e430f72e7325c43aba4d181517b9078dfc09637054",
                "f9dce3712dfa25c3ab516bef99c5e828b349e53a134321938612f5b5af9dd4bf",
                "74ef83448b8fc533a1dc33f1f58d1c0bd6cfb086f9369d627d917884ccc21da7",
                "63c0e43218a9e31e0ff5b34ab492a0e9fa100eebfc888c7efc51791a55cd8283",
                "9e0c36a0607c16d6d6284c640a8ac00ab65a772c3fa0981be022520a9f44e47a",
                "e288c14d070f463a7d71bc330b4786c574aa8be02f7e7ad00e4fc9a6bdcbb32b",
                "e758b66c5df5ef161f00081fe87685230a0b2846863cf3da00828d112b71220f",
                "1ed08c216fddc19622553925ef9d2753715c7c01201a9a47ae4233a4878cc030",
                "8e5bc962f77346e721f25fe7871a03e95bf38a86e564c5729c86dc2fc5eb6871",
                "68843c83a7c9829f6ef5c4ae77067f9d7f72703ee9b8c6d5498a873180b15574",
                "aee0085e8c98a2afbc7d43d5774e145cfa0168d9e22506dc79fa1b74718e0043",
                "e4c038f95dac570ffe276c16a3518983abcc8216e9d318de39ca42c70d54ea3e",
                "cb4f5cc032ca1e343229256bb0d911b0e04ec2f409d0fa837cedd973ac45b742",
                "1e63f2116013988df505b26ddebec1adaeb41c7a2b55baf375f599431b5fa242",
                "1792e2fed1c346900702dcc208e286ea8a6865975cd961c834a275c505072247",
                "a5672e32a331cc360317a7329fc7f70d7843586ab4a24b961336183cc764bef9",
                "52272eca966fe72012abffcc481276dce404c27e4db783374a5a7c001f9477d2",
                "98649a7620abd662e2ff699659f58b7cf19d9ad4e5257180cc36ba79fff97c68",
                "92dddc74b7491067fab985962c60b8e0ce43e820e35405f6ac2a577e4b6eca6e"
            ]
        },
        "ecdsa-v1": {}
    }
}

so in the oracle_event you have the descriptor and other metadata like times on one level and then you have a list of attestation schemes and their parameters. Note that nonces and public keys are always parameters to a certain attestation scheme (my oracle is still missing the public key which should be in there with the nonces).

Tibo-lg commented 2 years ago

@Tibo-lg @nkohen I believe this needs to be updated with the format in #163. We need to figure out which messages we want to keep as tlvs and what data structures we need to have keep the format in #163

Unless there is strong arguments, I think it'd be best to move everything to the same format as #163. I can update this PR if we agree on that (or probably make a new one as I'm not sure I can push to @nkohen branch).

Christewart commented 2 years ago

@Tibo-lg @nkohen I believe this needs to be updated with the format in #163. We need to figure out which messages we want to keep as tlvs and what data structures we need to have keep the format in #163

Unless there is strong arguments, I think it'd be best to move everything to the same format as #163. I can update this PR if we agree on that (or probably make a new one as I'm not sure I can push to @nkohen branch).

I think it comes down to this:

Do we expect announcements/attestations to be sent over the network directly to a peer, or do we always expected them to bet fetched from a 3rd party service like oracle.suredbits.com ? If we think the former, we should keep TLVs, if we think the latter, I think it might be ok to remove them (although I do think this makes parsing harder for the 3rd party service, there is no easy identifier for what the thing is being submitted to the service. A TLV is unique, whereas a subtype is not).

EDIT:

Thinking about this more, my own standard I laid out above is sort of violated already. We host contract_info on oracle.suredbits.com, for instance here is a list of numeric contract infos: https://oracle.suredbits.com/contract/numeric

Since contract_info no longer has a TLV prefix, they cannot be easily identified for parsing. In the case of contract info, we only support building them on the site (and not directly submitting them like we do oracle announcements/attesattions), so this is slightly different. Removing TLVs seems to really break down modularity and ease of parsing of independent pieces of the protocol.

Tibo-lg commented 2 years ago

Do we expect announcements/attestations to be sent over the network directly to a peer, or do we always expected them to bet fetched from a 3rd party service like oracle.suredbits.com ? If we think the former, we should keep TLVs, if we think the latter, I think it might be ok to remove them (although I do think this makes parsing harder for the 3rd party service, there is no easy identifier for what the thing is being submitted to the service. A TLV is unique, whereas a subtype is not).

I would think that we should at least plan for enabling peers to exchange announcements/attestations directly. But in that case I think they'd need to be in wire message format, not TLV?

Christewart commented 2 years ago

I'm not sure how you have implemented the wire message format, but in our code base the wire message format uses the nested TLV.

case class LnMessage[+T <: TLV](tlv: T) extends NetworkElement {
  require(tlv.tpe.toLong <= 65535L, s"LN Message format requires UInt16 types")
  val tpe: UInt16 = UInt16(tlv.tpe.toInt)
  val payload: ByteVector = tlv.value
  override lazy val bytes: ByteVector = tpe.bytes ++ payload
  val typeName: String = tlv.typeName
}

The key line is

val tpe: UIn16 = UInt16(tlv.tpe.toInt)

Since the announcement is no longer a TLV, it cannot have a tpe which is only defined for TLVs. How are you proposing we send non TLVs with the wire message format?

Tibo-lg commented 2 years ago

I'm not sure what you mean by "nested TLV"? The wire message doesn't have any length prefix and uses a u16 instead of a bigsize as written in the specs.

Christewart commented 2 years ago

the specs.

Meanwhile all typed sub-messages (which follow TLV format) will

^ What I am talking about. I can detail our implementation in Scala if you'd like but I think we have a misunderstanding of what the specification says. You adhere to this with all of the test vectors on #163 (which is why we are compatible), but want to break this convention -- IIUC -- on this PR.

If we are intending to send announcements/attestations over the wire independently they will need to be TLVs

Tibo-lg commented 2 years ago

Sorry I should have linked to the spec after #163 : https://github.com/Tibo-lg/dlcspecs-1/blob/serialization-update-proposal/Messaging.md#message-format

You adhere to this with all of the test vectors on https://github.com/discreetlogcontracts/dlcspecs/pull/163 (which is why we are compatible), but want to break this convention -- IIUC -- on this PR.

This is because you wanted to keep the oracle message format intact so as not to have to update your oracle infrastructure before this PR.

If we are intending to send announcements/attestations over the wire independently they will need to be TLVs

I don't see why. But maybe it will be easier to discuss it during the spec meeting? It feels indeed like we are not understanding each others.

Christewart commented 2 years ago

Sorry I should have linked to the spec after #163 : https://github.com/Tibo-lg/dlcspecs-1/blob/serialization-update-proposal/Messaging.md#message-format

You adhere to this with all of the test vectors on #163 (which is why we are compatible), but want to break this convention -- IIUC -- on this PR.

This is because you wanted to keep the oracle message format intact so as not to have to update your oracle infrastructure before this PR.

If we are intending to send announcements/attestations over the wire independently they will need to be TLVs

I don't see why. But maybe it will be easier to discuss it during the spec meeting? It feels indeed like we are not understanding each others.

A TLV stream is defined for each wire message

How are you suggesting to parse an announcement if it isn't a TLV? IIUC, we will have the length of the announcement, but no unique identifier? What is the unique identifier for an announcement so that I can identify it as an announcement before parsing the payload?

Perhaps it would be useful to make this more concrete, can you give me an example announcement you are envisioning in wire message format?

Tibo-lg commented 2 years ago

IIUC, we will have the length of the announcement, but no unique identifier? What is the unique identifier for an announcement so that I can identify it as an announcement before parsing the payload?

Wire messages have a u16 type prefix, isn't that enough?

Perhaps it would be useful to make this more concrete, can you give me an example announcement you are envisioning in wire message format?

There might be some things that need to be updated, but roughly the spec should look like it was before the commit that reverted the oracle related messages to the old format: https://github.com/discreetlogcontracts/dlcspecs/pull/163/commits/225e3b0693766c2d74442424249f83d044ef2316#diff-b8a9c2888fe05c198758fbe393798cd0802b578f7eb07dbf82cc2c90616f97c3R407

Concretely an announcement would be serialized similarly to an offer_dlc message, with first a u16 type prefix, and then the other fields (which wouldn't be TLVs as they are now in #163). When embedding an announcement inside an offer_dlc, I think we agreed that we would just keep the wire message format as is to make things simple, and so you'd have also the type prefix (which you wouldn't actually need in the context of the offer message), and again the other fields of announcement following.

Christewart commented 2 years ago

with first a u16 type prefix

IMO, you are using a TLV but just not calling it a TLV :-). Or perhaps my mental model of TLVs is incorrect. But functionally speaking, I think we are in agreement.

An announcement needs a type identifier similar to offer_dlc in 163 right?

Screenshot from 2022-06-06 06-37-11

https://github.com/discreetlogcontracts/dlcspecs/pull/163/files#diff-e0f5b925f91a1c09c6daf26f9a7d28816cb9ff9f08863faca719b7ee0a1cc065R67

If that is the case, I don't really care about definitions, we are referring to the same thing with different definitions. I'll do some research later to correct my definitions -- if they are wrong -- so we can use a common language to describe the protocol.

Tibo-lg commented 2 years ago

An announcement needs a type identifier similar to offer_dlc in 163 right?

Yes! The main difference between TLV and wire message format is that wire message don't have the length part (because you get that from the network layer already) and use u16 instead of bigsize for the type prefix.

Also note that wire messages include TLV extensions which might make things confusing (I'm not sure we want oracle messages to have that though, but it's a detail). If you think something could be made clearer in the specs let me know!

Christewart commented 2 years ago

I think the next question is how do announcements get serialized inside of other TLVs? I.e. offer_dlc has announcements inside of it, do you think it should be serialized in wire message format inside of the offer?

Christewart commented 2 years ago

Yes! The main difference between TLV and wire message format is that wire message don't have the length part (because you get that from the network layer already) and use u16 instead of bigsize for the type prefix.

What do you mean by "network layer"? Our length is serialized as part of the TLV, i.e. type length value.

Tibo-lg commented 2 years ago

I mean that the buffer you receive from the network socket should have a certain length, so you don't need a field to tell you the same thing. You should already be doing that for other messages like offer I think? (Also if the message is wellformed you don't even need that information, you'll just be parsing the content and be able to retrieve all the information you expect)

Christewart commented 2 years ago

Doesn't this fall apart when your messages are larger than a tcp frame? My understanding is we need things like #192 because our messages sizes may be too large, specifically with adaptor signatures. With TLVs, you know what the payload length should be as its given to you in the very first tcp frame.

Christewart commented 2 years ago

You should already be doing that for other messages like offer I think?

This might be the source of some misunderstanding on my part, we do infer the length as you say here:

https://github.com/bitcoin-s/bitcoin-s/blob/8c5288d75833d56d388247cdc928cf6694c46f46/core/src/main/scala/org/bitcoins/core/protocol/tlv/LnMessage.scala#L32

My comment above stands, but this does make more sense now

Tibo-lg commented 2 years ago

Doesn't this fall apart when your messages are larger than a tcp frame? My understanding is we need things like #192 because our messages sizes may be too large, specifically with adaptor signatures. With TLVs, you know what the payload length should be as its given to you in the very first tcp frame.

AFAIU the reason TLVs have a length field is so that you can ignore fields that you don't care or know about in extensions (and aren't mandatory). Having a length field in every single message might be one possible solution to message segmentation, though it wouldn't be my favorite because:

it would be useless in many cases
It would be more difficult to handle disconnects/reconnects (if I disconnect while sending a segmented message, and restart sending the same message upon reconnecting, the receiving peer will not notice)

Roughly what #192 does is to have a length field but only for messages that need them. There might be better ways to do it (and actually just re-looking at it now I feel it can be even simpler) but I think that's a discussion that we should have in #192 (and I'd be really happy to have it :) !).

Christewart commented 2 years ago

* it would be useless in many cases

This really isn't a compelling argument in my opinion, the point is that it is needed in some cases to have a coherent idea of the message that is being sent to you across the wire. Perhaps you have a deeper understanding of networking than I do, but I don't understand how the networking layer can have knowledge about our protocol?

* It would be more difficult to handle disconnects/reconnects (if I disconnect while sending a segmented message, and restart sending the same message upon reconnecting, the receiving peer will not notice)

I don't understand this. Why would I keep my peers bytes in my buffer if they disconnected from me? It could be days before they reconnect?

Roughly what #192 does is to have a length field but only for messages that need them. There might be better ways to do it (and actually just re-looking at it now I feel it can be even simpler) but I think that's a discussion that we should have in #192 (and I'd be really happy to have it :) !).

Ok, at least we are in agreement that a length field is needed for some messages.

Tibo-lg commented 2 years ago

Rebased on #163 and updated to match the new serialization @Christewart @nkohen please check.

@LLFourn I liked your idea of being able to support multiple attestation schemes, can you check if what I've done matches with what you had in mind?

LLFourn commented 2 years ago

@Tibo-lg I think that looks ok. The one thing I'd consider doing is putting the attestation key with the attestation scheme data itself. Probably "oracle_keys" should just have the announcement key and each attestation scheme inside each announcement should declare its attestation key. Each scheme should use a different attestation key. Clients could then by convention require that the same attestation key is used for every announcement (of the same scheme).

Tibo-lg commented 2 years ago

@Tibo-lg I think that looks ok. The one thing I'd consider doing is putting the attestation key with the attestation scheme data itself. Probably "oracle_keys" should just have the announcement key and each attestation scheme inside each announcement should declare its attestation key. Each scheme should use a different attestation key. Clients could then by convention require that the same attestation key is used for every announcement (of the same scheme).

Makes sense, updated.

Tibo-lg commented 2 years ago

I pushed updated test vectors including these oracle changes.

The questions that I have left:

Do we really need a timestamp for the metadata? (https://github.com/discreetlogcontracts/dlcspecs/pull/167#discussion_r896482864)
Do we need the oracle metadata in each announcement? (https://github.com/discreetlogcontracts/dlcspecs/pull/167#discussion_r896286653)
Are we sure we don't want to give proof of knowledge for attestation key and nonces?
(more cosmetic) Should we remove all the Oracle prefixes from the field names? It feels redundant to me (and I think we don't add them consistently).

Tibo-lg commented 2 years ago

Closes #183

Tibo-lg commented 2 years ago

Added proof of knowledge in the Schnorr scheme (@LLFourn when you have time to check)

Tibo-lg commented 2 years ago

Updated specs and test vectors to use regular Schnorr signatures for proof of knowledge as agreed during spec meeting (using a type prefix to enable using something different in the future).

discreetlogcontracts / dlcspecs

v0 Oracle Changes #167