discreetlogcontracts / dlcspecs

Specification for Discreet Log Contracts
Creative Commons Attribution 4.0 International
232 stars 36 forks source link

Update messaging and serialization #163

Closed Tibo-lg closed 2 years ago

Tibo-lg commented 3 years ago

This PR is a proposal to update the update the serialization to make it more efficient, concise and upgradable. The current message format specifies hard coded type prefixes for embedded type that don't require them, making it a maintenance burden and inefficient. It also lacks any extensibility or upgradability mechanism.

Changes include:

I hesitated to move the wire message definitions to Messaging.md as previously discussed but to keep the diff small I didn't.

Tibo-lg commented 2 years ago

Fixed the sibling type numbers which where not consistent with the latest update.

Tibo-lg commented 2 years ago

Noticed that we were sometimes using BigSize and sometimes u16 for collection prefixes. Consolidated it to use BigSize everywhere as for the CetSignatures type it is probably safer than having a restriction on size.

Tibo-lg commented 2 years ago
Tibo-lg commented 2 years ago

Added a commit that reverts the oracle messages to the previous version, and updated the test vectors.

Christewart commented 2 years ago

All contract ids are broken for existing DLCs by this change. Database migrations are required when implementing this change, else you will have invalid contract ids in your database that are no longer correct for this version of the specification.

Tibo-lg commented 2 years ago

All contract ids are broken for existing DLCs by this change. Database migrations are required when implementing this change, else you will have invalid contract ids in your database that are no longer correct for this version of the specification.

Should we remove the requirement that contract ID must be a hash of offer message? I don't recall the reason for choosing to do that originally, but it doesn't feel like bringing much and we might have this issue again in the future.

Christewart commented 2 years ago

All contract ids are broken for existing DLCs by this change. Database migrations are required when implementing this change, else you will have invalid contract ids in your database that are no longer correct for this version of the specification.

Should we remove the requirement that contract ID must be a hash of offer message? I don't recall the reason for choosing to do that originally, but it doesn't feel like bringing much and we might have this issue again in the future.

Perhaps. We needed to implement #143 in our codebase anyway which breaks contractIds as well (this is our fault for delaying implementation). Because of this i'll have to write the migration code regardless.

@matthewjablack what does this mean for your implementation? Do you guys use contractId quite a bit in your codebase?

EDIT: Here is our issue for this https://github.com/bitcoin-s/bitcoin-s/issues/3969#issue-1097199176

Tibo-lg commented 2 years ago

Rebased on master (realized it was missing the hyperbola update)

Tibo-lg commented 2 years ago

Updated to add the temporary_contract_id to the offer message as agreed during last spec meeting.

Christewart commented 2 years ago

So this PR on bitcoin-s passes all test vectors in this PR:

I believe we have achieved the 2 impl requirement, which means we may want to merge this? 👀 👀

https://github.com/bitcoin-s/bitcoin-s/pull/3859

Christewart commented 2 years ago

As discussed at the June meeting, we have 2 implementations now so merging this.