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

[DO NOT MERGE] fix: change scriptHash generation for spending condition #278

Closed
wants to merge 2 commits into from

Conversation

troggy
Copy link
Member

@troggy troggy commented Jun 14, 2019

@troggy
Copy link
Member Author

troggy commented Jun 14, 2019

integration tests need fixing 👀

@@ -227,7 +227,8 @@ module.exports = async (state, tx, bridgeState, nodeConfig = {}) => {
// we only allow one spending condition in an transaction, do we want to throw if we find more?
spendingInput = input;
spendingInputUnspent = unspent;
spendingAddrBuf = utils.ripemd160(spendingInput.script);
const { script } = spendingInput;
spendingAddrBuf = utils.ripemd160(script.length, utils.keccak256(script));
Copy link
Contributor

Choose a reason for hiding this comment

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

How does ethereumjs-util handle the int in ripemd, does it pad it somehow to a fixed size integer?

Copy link
Member Author

Choose a reason for hiding this comment

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

🤦‍♂ sorry, it is all broken .. ripemd doesn't even accepts multiple args as inputs. Will fix this mess tomorrow

Copy link
Member Author

@troggy troggy Jun 14, 2019

Choose a reason for hiding this comment

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

should be really something like ripemd160([keccak256(length), keccak256(script)])

Should we pad the ripemd hash itself as well? Though it will always be padded with 12 zero bytes

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we pad the ripemd hash itself as well? Though it will always be padded with 12 zero bytes

No need to pad the ripemd hash.

Here is the current implementation: https://github.com/leapdao/leap-core/pull/114/files#diff-7e1ea5f3cb35910ce52c225628d785d7R4

We can just pad script.length with zero-bytes on the left side to 32 bytes like
0x${script.length.toString(16).padStart(64, '0')}
That way we have a constant-size variable and everything is nice :)

@codecov
Copy link

codecov bot commented Jun 15, 2019

Codecov Report

Merging #278 into master will decrease coverage by 20.76%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #278       +/-   ##
==========================================
- Coverage   94.57%   73.8%   -20.77%     
==========================================
  Files          62      62               
  Lines         922     924        +2     
  Branches      173     170        -3     
==========================================
- Hits          872     682      -190     
- Misses         46     197      +151     
- Partials        4      45       +41
Impacted Files Coverage Δ
src/tx/applyTx/checkSpendCond.js 0% <0%> (-91.92%) ⬇️
src/api/methods/checkSpendingCondition.js 0% <0%> (-100%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48c234c...bb63bef. Read the comment docs.

@troggy troggy changed the title fix: change scriptHash generation for spending condition [DO NOT MERGE] fix: change scriptHash generation for spending condition Jun 19, 2019
@troggy
Copy link
Member Author

troggy commented Sep 12, 2019

stale

@troggy troggy closed this Sep 12, 2019
@troggy troggy deleted the fix/242-spencon-address branch January 20, 2020 06:26
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

Successfully merging this pull request may close these issues.

SpendingCondition: use ripemd160(length, sha3(bytecode)) instead of just using ripemd160
2 participants