grammar fixes BSIP 44 (HTLC) by jmjatlanta · Pull Request #112 · bitshares/bsips

jmjatlanta · 2018-10-12T22:12:54Z

This PR fixes a few "bugs" in the HTLC BSIP.

Comments from @abitmore that require more discussion:

Specifically:

Whether (and when) HTLCs should obey whitelisting/blacklisting
Risk of exposing preimage if there is a badly-timed chain reorganization or transaction censoring
How the summary should be worded (shortened).

Please discuss the changes made, Abit's concerns, and anything else you see that should be fixed below.

pmconrad · 2018-10-13T08:19:05Z

Please specify initially supported hash algorithms
Please add new chain parameter 'max_htlc_preimage_length' and default value to specification section

abitmore · 2018-10-13T22:30:35Z

Please add new chain parameter 'max_htlc_preimage_length' and default value to specification section

preimage_size is defined as uint16_t, which implies that the maximum length should be 65535 or less.

jmjatlanta · 2018-10-15T10:43:28Z

Please add new chain parameter 'max_htlc_preimage_length' and default value to specification section

Suggestions as to what that limit should be? I can increase the variable size, if needed. I think 65K is plenty big for now. If people desire to use big stuff for preimages (i.e. real images) it could be changed later. My guess is most will use random gobldygook generated by their software.

abitmore · 2018-10-15T10:49:38Z

I think we'd better implement bitshares/bitshares-core#167 before adding new parameters to global parameters (which needs a new BSIP as well), although it's possible to add new parameters now via extensions (see cryptonomex/graphene#612).

Probably we can port some code from YOYOW.

jmjatlanta · 2018-10-15T11:55:55Z

I went ahead and added some limits, just to see what you think. The BSIP still says that the committee should set the max timeout and max preimage size. So we need to pick hardcode vs. committee.

I agree that bitshares/bitshares-core/#167 should be implemented. I would rather do that than shoe-horn in a parameter via an extension.

So, I am looking for suggestions:

Hard-code values (at least for now), and not permit the committee to change it.
Add via extensions
Push a BSIP for Onchain margin position management #167 and get that in first.

pmconrad · 2018-10-15T12:51:47Z

IMO it makes little sense to use a preimage that is longer than the hash size. I'd suggest a default maximum of 1024 bytes. Maybe even less.

I think HTLC is more imporant than bitshares/bitshares-core#167, so we shouldn't create a dependency there. Still, the parameter should be a chain_parameter right from the start. I think adding the extension would be the best option.

jmjatlanta · 2018-10-15T13:18:36Z

IMO it makes little sense to use a preimage that is longer than the hash size. I'd suggest a default maximum of 1024 bytes. Maybe even less.

I think HTLC is more imporant than bitshares/bitshares-core#167, so we shouldn't create a dependency there. Still, the parameter should be a chain_parameter right from the start. I think adding the extension would be the best option.

All that makes sense. My desire to not do things twice was greater than what is logical. Updated spec to eliminate the hard-coding and went back to committee flexibility.

jmjatlanta · 2019-01-03T19:57:57Z

I have modified the document to more closely match the implementation, and to make terminology consistent. If @abitmore or @pmconrad could please give it a second look to verify its accuracy. Thanks!

pmconrad · 2019-01-07T19:55:45Z

bsip-0044.md

-         asset                   pending_fee;
-         vector<unsigned char>   preimage_hash;
-         fc::enum_type<uint8_t, hash_algorithm> preimage_hash_algorithm;
+         htlc_hash               preimage_hash;


need to define htlc_hash type if you mention it here

pmconrad · 2019-01-07T19:56:21Z

bsip-0044.md

  Set: `contract.preimage_length` = `preimage_length`
  Set: `contract.timeout_treshold` = `timeout_threshold`
-  Transfer: from `depositor` account to `contract.quantity` of `contract.symbol`
+  Transfer: remove `contract.quantity` of `contract.symbol` from from` account 


Missing backtick before second "from"

pmconrad · 2019-01-07T19:59:29Z

bsip-0044.md

+   };
 ```

+Note 1: The preimage hash algorithm must be speceified as  either ``SHA256``, ``RIPEMD160``, or ``SHA1``. The use of ``SHA1`` is added for compatibility to other chains, but its use is discouraged.


This note is no longer applicable.

pmconrad · 2019-01-07T20:09:28Z

Generally, I think BSIPs should not be modified after approval. Modifying implementation details is probably ok to prevent future confusion.

This PR contains a little more than just "grammar fixes" and implementation details. I think these additional changes do not modify the original intent and are therefore probably ok too.

pmconrad · 2019-01-07T20:11:49Z

bsip-0044.md

+   typedef fc::static_variant<
+      htlc_algo_ripemd160,
+      htlc_algo_sha1,
+      htlc_algo_sha256


Now you should mention what these three new types are... :-)

Do you think it best to put it in a descriptive note below this section or a //comment on each line? I'll give //comment a try.

pmconrad

Ok for me.
Would like a second opinion though. @sschiessl-bcp perhaps?

ryanRfox

Thank you @jmjatlanta for making all the changes requested by @abitmore and @pmconrad . This PR is ready for merge.

grammar fixes

6dda715

Added some parameter limits

6efbbc2

jmjatlanta changed the title ~~grammar fixes~~ grammar fixes BSIP 44 (HTLC) Oct 15, 2018

Re-added committee parameters

57f21fa

Updated to match implementation more closely

bb00710

pmconrad reviewed Jan 7, 2019

View reviewed changes

punctuation fixes and clarifications

cf4206d

pmconrad reviewed Jan 7, 2019

View reviewed changes

jmjatlanta added 2 commits January 7, 2019 15:22

added comments about htlc_hash algos

9664001

better spacing in comments

6f72b4b

pmconrad approved these changes Jan 8, 2019

View reviewed changes

ryanRfox self-requested a review January 8, 2019 18:02

ryanRfox approved these changes Jan 8, 2019

View reviewed changes

ryanRfox merged commit a225d63 into master Jan 8, 2019

abitmore deleted the 44_adjustments branch May 21, 2019 16:43

Conversation

jmjatlanta commented Oct 12, 2018

Uh oh!

pmconrad commented Oct 13, 2018

Uh oh!

abitmore commented Oct 13, 2018

Uh oh!

jmjatlanta commented Oct 15, 2018

Uh oh!

abitmore commented Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmjatlanta commented Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pmconrad commented Oct 15, 2018

Uh oh!

jmjatlanta commented Oct 15, 2018

Uh oh!

jmjatlanta commented Jan 3, 2019

Uh oh!

pmconrad Jan 7, 2019 • 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!

pmconrad Jan 7, 2019

Choose a reason for hiding this comment

Uh oh!

pmconrad Jan 7, 2019

Choose a reason for hiding this comment

Uh oh!

pmconrad commented Jan 7, 2019

Uh oh!

pmconrad Jan 7, 2019

Choose a reason for hiding this comment

Uh oh!

jmjatlanta Jan 7, 2019

Choose a reason for hiding this comment

Uh oh!

pmconrad left a comment

Choose a reason for hiding this comment

Uh oh!

ryanRfox left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

abitmore commented Oct 15, 2018 •

edited

Loading

jmjatlanta commented Oct 15, 2018 •

edited

Loading

pmconrad Jan 7, 2019 •

edited

Loading