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.

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

BSIP 64: Optional HTLC Preimage Length and Add Hash160 Algorithm #163

Closed
abitmore opened this issue May 1, 2019 · 25 comments
Closed

BSIP 64: Optional HTLC Preimage Length and Add Hash160 Algorithm #163

abitmore opened this issue May 1, 2019 · 25 comments

Comments

@abitmore
Copy link
Member

abitmore commented May 1, 2019

BSIP: 64
Title: Optional HTLC preimage length and HASH160 addition
Authors: John Jones <jmjatlanta@gmail.com>, abitmore
Status: Draft
Type: Protocol
Created: 2019-07-09
Discussion: https://github.com/bitshares/bsips/issues/163

Abstract

This BSIP proposes two changes to Hashed Time Lock Contracts (HTLC) defined in BSIP 44. The first makes the preimage length parameter optional. The second adds the HASH160 algorithm to the set of allowed hash functions.

Motivation

The original implementation of Hashed Timelock Contracts included a protection to prevent an attack based on the size of the preimage. While it is still believed this is a valuable feature, this makes the implementation incompatible with a strict implementation of the popular BIP199 implemented by Bitcoin.

In addition, BIP199 includes the ability to use HASH160, which is not currently part of the BitShares implementation.

Risks

Note below is a discussion of HTLC risks in general, not risks inherent in implementing this BSIP.

An Oversized Preimage Attack There are limits to the size of a preimage that can be used. That limit is set by the blockchain itself. But when dealing with atomic swaps across blockchains, an attacker could use a preimage that is too large for one side of the 2 sided transaction. That means that an HTLC can be created on both chains, but only redeemed on one chain, and not the other.

This can be mitigated by specifying the preimage size as part of the contract. This feature is available on many bitcoin-based chains among others, but is not standard. Should the preimage size be used with an atomic swap, both sides of the transaction should include the preimage size in their contract.

Timelock Attacks It is standard practice for HTLCs that the timelock should be long enough so that the block that contains the contract can be considered irreversible. This makes it possible for the redeemer to expose the preimage and still be guaranteed that the contract itself will not be reversed.

This becomes even more important with atomic swaps. The shorter duration (a.k.a “inner”) contract should allow time to achieve its own irreversibility. And the longer duration (a.k.a. “outer”) contract must allow time for irreversibility of both.

In addition, with an atomic swap both sides must consider the redemption time necessary. The creator of the inner contract must decide that should the redeemer redeem at the last moment, is there enough time to redeem the outer contract before the timelock expires.

One final consideration of the timelock portion of the contract is the use of capital. Should the other party not accept, the funds in the contract are held until the timelock expires. Long expiration times could result in missed opportunity costs.

Specifications

Technically, the impelentation of these changes are small.

Preimage Size: Permit that an HTLC can have the preimage size set to 0. This removes the size check during validation.

HASH160: Include this additional hashing algorithm to the list of hashes available to be used.

Discussion

#163

Copyright

This document is placed in the public domain.

See Also

Bitshares BSIP 44
Bitcoin BIP 199
Preimage Size Attack

@syalon
Copy link
Member

syalon commented May 1, 2019

In the case of ignoring the extremely low probability of hash collisions, omitting preimage length is much more convenient for the user. I think this improvement is necessary.

@jmjatlanta
Copy link
Contributor

Under the category of "ease of use" I believe we also should include HASH160 as an available hash. Would this require a bsip as well? Is it too different to put it with this bsip?

@ryanRfox
Copy link
Contributor

ryanRfox commented May 1, 2019

I do not believe adding HASH160 as an available hash would require a distinct BSIP, as the HTLC BSIP makes clear we intend to support relevant hash algs over time. IMO, same would apply to remove a broken alg. I consider both cases implementation details. Suggest to open an Issue within Core.

@ryanRfox
Copy link
Contributor

ryanRfox commented May 1, 2019

I'm open to revising the HTLC BSIP to make preimage_length optional. Beyond the collision protection properties, it also informs the redeeming party what the total fees will be to complete the transaction. Optional, but encouraged works for me. I feel we can author a new BSIP for this change, get it approved, then revise BSIP44 to include the change with references.

@pmconrad
Copy link
Contributor

pmconrad commented May 2, 2019

ISTR we talked about compatibility with other chains when we were discussing recommendations for the committee.
IIRC the preimage length was considered an important piece of information because very long preimages might be prohibitive for one chain but not another. Leaving it out would open up a way to scam people.
It was also mentioned that the preimage length is supported by other blockchains as well, although not required nor used in standard examples, but that using it is considered "best practice". @jmjatlanta can you confirm, perhaps with a quote? Or am I wrong?

Adding another hash algorithm is a protocol change and would IMO require a BSIP. Of course it makes sense to bundle all planned changes to HTLC into one BSIP instead of writing many little ones.

@ryanRfox
Copy link
Contributor

ryanRfox commented May 2, 2019

@pmconrad I'm actually not aware of any HTLC implementations that include hash length. I included in the specification for the reasons you stated above. My desire is for BitShares to improve upon the design bitcoin implemented. I don't want to hinder adoption by making it required. It should be highly recommended (within our documentation) to mitigate risk. I feel it use should become "best practice" going forward.

@ryanRfox
Copy link
Contributor

ryanRfox commented Jun 6, 2019

I feel BSIP44 should be redefined, rather than creating a new BSIP here. It's clear the original design requiring the preimage_length is limiting adoption of our HTLC implementation. As one of the authors on BSIP44, I must admit it contains a design bug. It must be redefined to be an optional field to be compatible with existing HTLC implementations such as BIP199.

@pmconrad
Copy link
Contributor

pmconrad commented Jun 7, 2019

I feel BSIP44 should be redefined, rather than creating a new BSIP here.

I disagree. A BSIP that has been approved by voters should not be changed anymore.

A modification to the original design is a new change that should be described and approved separately. (Precedent: RFCs)

Content-wise I still think a mandatory preimage length is a good idea because it prevents certain ways to scam people. (Admittedly, the mandatory length open certain other ways to scam careless people. The important difference is that with the mandatory length careful people can protect themselves whereas without they can't.)

Perhaps we should compile an overview

  • which chains support HTLC with (perhaps optional) preimage length
  • which 3rd parties support (perhaps optional) preimage length
  • which chains and 3rd parties support neither and thus can't interoperate with ours

@ryanRfox
Copy link
Contributor

ryanRfox commented Jun 7, 2019

A BSIP that has been approved by voters should not be changed anymore.

I will support this and retract my prior statement. If a new BSIP is approved, status of BSIP44 would become superseded. This seems the best case.

The only site I am aware of tracking HTLC (and PC) compatibility is https://swapready.net/ However, it does not capture preimage length requirements (optional). We may need to make our own.

@jmjatlanta
Copy link
Contributor

jmjatlanta commented Jul 10, 2019

If this meets your requirements @abitmore, I can move it to the top of this issue...

BSIP: TBD
Title: HTLC Compatibility Changes
Authors: John Jones <jmjatlanta@gmail.com>, abitmore
Status: Draft
Type: Protocol
Created: 2019-07-09
Discussion: https://github.com/bitshares/bsips/issues/163

Abstract

This BSIP proposes two changes to Hashed Time Lock Contracts (HTLC) defined in BSIP 44. The first makes the preimage length parameter optional. The second adds the HASH160 algorithm to the set of allowed hash functions.

Motivation

The original implementation of Hashed Timelock Contracts included a protection to prevent an attack based on the size of the preimage. While it is still believed this is a valuable feature, this makes the implementation incompatible with a strict implementation of the popular BIP199 implemented by Bitcoin.

In addition, BIP199 includes the ability to use HASH160, which is not currently part of the BitShares implementation.

Risks

Note below is a discussion of HTLC risks in general, not risks inherent in implementing this BSIP.

An Oversized Preimage Attack There are limits to the size of a preimage that can be used. That limit is set by the blockchain itself. But when dealing with atomic swaps across blockchains, an attacker could use a preimage that is too large for one side of the 2 sided transaction. That means that an HTLC can be created on both chains, but only redeemed on one chain, and not the other.

This can be mitigated by specifying the preimage size as part of the contract. This feature is available on many bitcoin-based chains among others, but is not standard. Should the preimage size be used with an atomic swap, both sides of the transaction should include the preimage size in their contract.

Timelock Attacks It is standard practice for HTLCs that the timelock should be long enough so that the block that contains the contract can be considered irreversible. This makes it possible for the redeemer to expose the preimage and still be guaranteed that the contract itself will not be reversed.

This becomes even more important with atomic swaps. The shorter duration (a.k.a “inner”) contract should allow time to achieve its own irreversibility. And the longer duration (a.k.a. “outer”) contract must allow time for irreversibility of both.

In addition, with an atomic swap both sides must consider the redemption time necessary. The creator of the inner contract must decide that should the redeemer redeem at the last moment, is there enough time to redeem the outer contract before the timelock expires.

One final consideration of the timelock portion of the contract is the use of capital. Should the other party not accept, the funds in the contract are held until the timelock expires. Long expiration times could result in missed opportunity costs.

Specifications

Technically, the impelentation of these changes are small.

Preimage Size: Permit that an HTLC can have the preimage size set to 0. This removes the size check during validation.

HASH160: Include this additional hashing algorithm to the list of hashes available to be used.

Discussion

#163

Copyright

This document is placed in the public domain.

See Also

Bitshares BSIP 44
Bitcoin BIP 199
Preimage Size Attack

@pmconrad
Copy link
Contributor

This is now duplicated by #174 - which one should we close / where to continue the discussion?

@jmjatlanta please add a "Risks" section describing attack vectors with and without preimage length. Also note that other chains may have implicit limits that are not well-defined, e. g. through maximum transaction length.

@jmjatlanta
Copy link
Contributor

Well, @abitmore won as his issue number is lower. Do you agree @ryanRfox ? I say we merge the two by copying or linking any important comments in the other to this one.

And, yes, I believe a "Risks" session is in order. I will add it.

@ryanRfox
Copy link
Contributor

Yes, let's merge the relevant bits from here, then close.

@jmjatlanta
Copy link
Contributor

jmjatlanta commented Aug 7, 2019

Interesting post from merged issue:

Does declaring preimage length reduce collision risk?

@jmjatlanta
Copy link
Contributor

jmjatlanta commented Aug 7, 2019

I would like to ask @abitmore as the OP if it would be okay for me to change the title to BSIP64: Hashed Time-Locked Contract and move the above post to the top.

Also, please edit or let me know if someone should be added to the authors list.

@abitmore
Copy link
Member Author

abitmore commented Aug 8, 2019

I think it's better (readability) to have the title and top post (OP) expresses what piece we're changing, than to show the whole thing after changed. It would be more friendly to readers.

@abitmore
Copy link
Member Author

abitmore commented Aug 8, 2019

@jmjatlanta the above post is fine to updated to OP. Title should be updated to "BSIP64: something" but I think "BSIP64: Hashed Time-Locked Contract " is not a good one.

@abitmore
Copy link
Member Author

abitmore commented Aug 8, 2019

"Title: HTLC Compatibility Changes" is not clear enough as a title. We have enough room to add a brief about the changes in the title.

@jmjatlanta jmjatlanta changed the title New BSIP: optional HTLC preimage length BSIP 64: optional HTLC preimage length and HASH160 addition Aug 9, 2019
@jmjatlanta
Copy link
Contributor

Modified title and original post. The old OP is below:

HTLC of some (or all?) other blockchains has no preimage length parameter. To be compatible we can effectively let it be optional, e.g. don't check the length when redeeming if the HTLC's preimage_length field is zero.

Related discussion: bitshares/bitshares-core#1713 (comment)

@syalon would this work for you?

@abitmore
Copy link
Member Author

abitmore commented Aug 9, 2019

@jmjatlanta please update the text in OP, add BSIP number and update the "title" in the text.

Why not create a pull request?

@ryanRfox ryanRfox changed the title BSIP 64: optional HTLC preimage length and HASH160 addition BSIP 64: Optional HTLC Preimage Length and Add Hash160 Algorithm Aug 21, 2019
@ryanRfox
Copy link
Contributor

Core Team today discussed removing SHA1 from the list of supported hash functions, as its crypto is considered "broken". The use of SHA1 on bitcoin and other networks is already discouraged. The BitShares project intends to mitigate risks to end users by implementing good crypto best practices. We propose the removal of SHA1 from the implementation by revising the BSIP64 spec.

@ioBanker
Copy link
Member

Please add the support of ed25519-sha-256 since it's so important when we later start creating atomic swaps compatible with XRP network.

@ioBanker
Copy link
Member

@ryanRfox
Copy link
Contributor

ryanRfox commented Oct 1, 2019

@ioBanker I too am keen to support additional hash algos to facilitate cross-chain communications. Unfortunately, at this time we must defer this change request to add ed25519-sha-256 into a future BSIP draft, as we desire to place BSIP64 for voting as is to allow us the best opportunity to make the 4.0.0 Protocol Release if approved.

I encourage you to begin that process now by using BSIP64 as a template for a new draft BSIP to add ed25519-sha-256 into our HTLC protocol. Just create a new Issue, paste in the BSIP text to the description and modify the relevant sections. Thanks for your efforts.

@abitmore
Copy link
Member Author

The draft has been written and merged, see #195.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants