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

Add Hash160 algorithm #143

Merged
merged 8 commits into from Apr 15, 2020
Merged

Add Hash160 algorithm #143

merged 8 commits into from Apr 15, 2020

Conversation

jmjatlanta
Copy link

@jmjatlanta jmjatlanta commented Jul 24, 2019

This algorithm is used by Bitcoin's BSIP199 for Hashed Time Lock Contracts. This is part of BitShares BSIP64

I attempted to remove the impl, as I am not using it. Unfortunately things will not compile without it, and I am not familiar with the templates that answer "why" this is the case. I can leave it as is, move the SHA256 stuff into the impl, or spend more time digging into the "why" and see what I can do.

Suggestions appreciated.

@pmconrad
Copy link

I attempted to remove the impl, as I am not using it.

AFAICS the only place where encoder::impl is used is here: https://github.com/bitshares/bitshares-fc/pull/143/files#diff-c871c59a37683d8807f37150fab164eeR68
my in turn doesn't seem to be used anywhere. fc::fwd only reserves sufficient space for impl - the idea is that it can be stack-allocated without knowing the full declaration of impl.
Did I miss anything?

@jmjatlanta
Copy link
Author

jmjatlanta commented Sep 20, 2019

This PR will help with the HTLC improvements BSIP that will hopefully be accepted.

https://github.com/bitshares/bsips/blob/master/bsip-0064.md

@abitmore abitmore changed the base branch from master to for-core-4.0.x March 31, 2020 15:42
src/crypto/hash160.cpp Outdated Show resolved Hide resolved
@abitmore
Copy link
Member

There are new changes on the ripemd160 files after you created your fork, please apply the same changes to your new class. Thanks.

@jmjatlanta
Copy link
Author

There are new changes on the ripemd160 files after you created your fork, please apply the same changes to your new class. Thanks.

Done.

Copy link
Member

@abitmore abitmore left a comment

Choose a reason for hiding this comment

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

It seems your encoder implementation lacks of a reset() function, which means callers need to multiple encoder instances for encoding multiple items.

@jmjatlanta
Copy link
Author

It seems your encoder implementation lacks of a reset() function, which means callers need to multiple encoder instances for encoding multiple items.

Was in header, but forgot to implement. Nice catch.

@abitmore
Copy link
Member

I don't know why my is needed for compile, and don't want to spend more time researching it at this moment. To avoid unexpected trouble, please update your code to be inline with other classes, move the implementation details to the impl class. Thanks.

@abitmore
Copy link
Member

Thanks.

@abitmore abitmore merged commit 73a7f08 into for-core-4.0.x Apr 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants