Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

grammar fixes BSIP 44 (HTLC) #112

Merged
merged 7 commits into from
Jan 8, 2019
Merged

grammar fixes BSIP 44 (HTLC) #112

merged 7 commits into from
Jan 8, 2019

Conversation

jmjatlanta
Copy link
Contributor

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

Comments from @abitmore that require more discussion:

#104 (review)

Specifically:

  1. Whether (and when) HTLCs should obey whitelisting/blacklisting
  2. Risk of exposing preimage if there is a badly-timed chain reorganization or transaction censoring
  3. 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
Copy link
Contributor

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

@abitmore
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

abitmore commented Oct 15, 2018 •
edited
Loading

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
Copy link
Contributor Author

jmjatlanta commented Oct 15, 2018 •
edited
Loading

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:

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

@jmjatlanta jmjatlanta changed the title grammar fixes grammar fixes BSIP 44 (HTLC) Oct 15, 2018
@pmconrad
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
pmconrad reviewed Jan 7, 2019
View reviewed changes
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;
Copy link
Contributor

@pmconrad pmconrad Jan 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to define htlc_hash type if you mention it here

pmconrad
pmconrad reviewed Jan 7, 2019
View reviewed changes
bsip-0044.md Outdated
Set: `contract.hash_algorithm` = `hash_algorithm`
Set: `contract.preimage_hash` = `preimage_hash`
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing backtick before second "from"

pmconrad
pmconrad reviewed Jan 7, 2019
View reviewed changes
bsip-0044.md Outdated
```

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This note is no longer applicable.

@pmconrad
Copy link
Contributor

pmconrad commented Jan 7, 2019

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
pmconrad reviewed Jan 7, 2019
View reviewed changes
bsip-0044.md Outdated
typedef fc::static_variant<
htlc_algo_ripemd160,
htlc_algo_sha1,
htlc_algo_sha256
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
pmconrad approved these changes Jan 8, 2019
View reviewed changes
Copy link
Contributor

@pmconrad pmconrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ryanRfox ryanRfox self-requested a review January 8, 2019 18:02
ryanRfox
ryanRfox approved these changes Jan 8, 2019
View reviewed changes
Copy link
Contributor

@ryanRfox ryanRfox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ryanRfox ryanRfox merged commit a225d63 into master Jan 8, 2019
@abitmore abitmore deleted the 44_adjustments branch May 21, 2019 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmconrad pmconrad pmconrad approved these changes

@ryanRfox ryanRfox ryanRfox approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jmjatlanta @pmconrad @abitmore @ryanRfox