Skip to content

Conversation

@mvandeberg
Copy link
Contributor

Closes #1944

@theoreticalbts
Copy link
Contributor

Looks reasonable to me.

@mvandeberg mvandeberg force-pushed the 1944-enforce-canonical-signatures branch from b90f5b6 to 414ac3f Compare June 21, 2018 18:04
@mvandeberg mvandeberg force-pushed the 1944-enforce-canonical-signatures branch from 9af0755 to 63fc2a8 Compare June 21, 2018 20:43
goldibex
goldibex previously approved these changes Jul 2, 2018
Copy link

@goldibex goldibex left a comment

Choose a reason for hiding this comment

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

In general -- reading this PR makes me realize we need a way to test transitions across hardforks. It would be really nice to have a test that

  • tries to submit a transaction with a non-FC-canonical but BIP-0062-canonical signature pre-fork and asserts failure
  • joins HF20
  • submits the transaction again and asserts success

const authority_getter& get_owner,
const authority_getter& get_posting,
uint32_t max_recursion = STEEM_MAX_SIG_CHECK_DEPTH
uint32_t max_recursion/* = STEEM_MAX_SIG_CHECK_DEPTH*/,

Choose a reason for hiding this comment

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

Please just delete these defaults rather than comment them out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left the defaults there because there is some cleanup that needs occur post HF that will cleanup this interface and restore it to its pre hardfork state.

for( size_t i = 0; i < 4; i++ )
{
sig <<= 64;
sig += boost::endian::big_to_native( *( uint64_t* )( c.data + 33 + ( i * 8 ) ) );
Copy link

Choose a reason for hiding this comment

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

I'm not convinced all this complexity is necessary. Since the numbers are both guaranteed to be big-endian and fixed-length, a simple memcmp should be all that's necessary to determine which is the larger number.

@goldibex goldibex dismissed their stale review July 2, 2018 16:24

whoops

Copy link

@goldibex goldibex left a comment

Choose a reason for hiding this comment

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

See previous comments. memcmp or the equivalent c++ sugar is the easiest way to verify the supplied signature is smaller than N/2.

@goldibex
Copy link

goldibex commented Jul 5, 2018

lgtm

@mvandeberg mvandeberg merged commit 8e691dd into develop Jul 5, 2018
@mvandeberg mvandeberg deleted the 1944-enforce-canonical-signatures branch July 5, 2018 16:43
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.

4 participants