Skip to content

Split-out Jupyter Chapter 0.2 (EC Math Review) from Chapter 1.1 (Schnorr Introduction). - #13

Merged
jnewbery merged 5 commits into
bitcoinops:masterfrom
jachiang:PR-13
Sep 16, 2019
Merged

Split-out Jupyter Chapter 0.2 (EC Math Review) from Chapter 1.1 (Schnorr Introduction).#13
jnewbery merged 5 commits into
bitcoinops:masterfrom
jachiang:PR-13

Conversation

@jachiang

@jachiang jachiang commented Sep 3, 2019 •
edited by jnewbery
Loading

Copy link
Copy Markdown
Contributor

As discussed, the EC math section has been split out from Chapter 1.1 (Schnorr Introduction), and is now considered workshop preparation along with chapter 0.1.

This PR incorporates some of the feedback from Elichai's review of Chapter 1.1.

  • SECP256K1_ORDER corrections - here
  • Correction of quadratic residue mod field order - here
  • Unwieldy ECKey.set(int.to_bytes()) not addressed yet - here

I will wait for @bitschmidty review of chapter 0.2 and 1.1 before tackling unaddressed todo's above.

@jachiang jachiang mentioned this pull request Sep 3, 2019
Closed
jnewbery
jnewbery reviewed Sep 4, 2019
View reviewed changes
Comment thread 0.2-Elliptic-Curve-Math.ipynb
Comment thread 0.2-Elliptic-Curve-Math.ipynb Outdated
Comment thread 0.2-Elliptic-Curve-Math.ipynb Outdated
Comment thread 0.2-Elliptic-Curve-Math.ipynb Outdated
Comment thread 0.2-Elliptic-Curve-Math.ipynb Outdated
Comment thread 0.2-Elliptic-Curve-Math.ipynb Outdated
Comment thread 0.2-Elliptic-Curve-Math.ipynb
Comment thread 0.2-Elliptic-Curve-Math.ipynb
Comment thread 0.2-Elliptic-Curve-Math.ipynb Outdated
Comment thread 0.2-Elliptic-Curve-Math.ipynb Outdated
@jnewbery

jnewbery commented Sep 4, 2019

Copy link
Copy Markdown
Contributor

rebased on master

bitschmidty
bitschmidty reviewed Sep 13, 2019
View reviewed changes

@bitschmidty bitschmidty left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good overall @jachiang !

I tried to help implement some of Johns feedback in my comments.

Comment thread 0.2-Elliptic-Curve-Math.ipynb Outdated
Comment thread 0.2-Elliptic-Curve-Math.ipynb Outdated
Comment thread 0.2-Elliptic-Curve-Math.ipynb
Comment thread 0.2-Elliptic-Curve-Math.ipynb Outdated
"* Multiplication: `mul`\n",
"* Division: `mul` (multiplying with scalar inverse, see Fermat's little theorem) \n",
"\n",
"![test](images/ec_math1.jpg)"

@bitschmidty bitschmidty Sep 13, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Using int here is confusing for me. int+int, can it just be a+b, like the examples in the preceding bullet examples?

@jnewbery jnewbery Sep 16, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

still not addressed

Comment thread 0.2-Elliptic-Curve-Math.ipynb Outdated
Comment thread 0.2-Elliptic-Curve-Math.ipynb
Comment thread 0.2-Elliptic-Curve-Math.ipynb Outdated
Comment thread Solutions/0.2-Elliptic-Curve-Math-Solutions.ipynb Outdated
Comment thread Solutions/0.2-Elliptic-Curve-Math-Solutions.ipynb Outdated
Comment thread 0.2-Elliptic-Curve-Math.ipynb Outdated
@jachiang
Adapted chapter for __add__/__mul__ key methods.
b737343 
Also includes more explanations on scalar/points and additional exercise
(commutivity).
@jachiang

jachiang commented Sep 15, 2019 •
edited by jnewbery
Loading

Copy link
Copy Markdown
Contributor Author

Updated incorporating feedback from @jnewbery @elichai. Requires this PR branch to run.

TODO:

@jnewbery

jnewbery commented Sep 15, 2019

Copy link
Copy Markdown
Contributor

@jnewbery @jachiang Are we having them do this prework exercises at home? Or are the "examples" to be done at home as prework and the "exercises" to be done when reviewing during the workshop?

I think it'd be ideal if the attendees did all the prework at home, and then we go over the solutions on the day.

jnewbery
jnewbery reviewed Sep 15, 2019
View reviewed changes

@jnewbery jnewbery left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left a bunch of nits

Comment thread 0.2-Elliptic-Curve-Math.ipynb Outdated
Comment thread 0.2-Elliptic-Curve-Math.ipynb Outdated
Comment thread 0.2-Elliptic-Curve-Math.ipynb Outdated
Comment thread 0.2-Elliptic-Curve-Math.ipynb Outdated
Comment thread 0.2-Elliptic-Curve-Math.ipynb Outdated
Comment thread 0.2-Elliptic-Curve-Math.ipynb Outdated
"* Multiplication: `mul`\n",
"* Division: `mul` (multiplying with scalar inverse, see Fermat's little theorem) \n",
"\n",
"![test](images/ec_math1.jpg)"

@jnewbery jnewbery Sep 15, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

still not addressed

Comment thread 0.2-Elliptic-Curve-Math.ipynb Outdated
"* Multiplication: `a * b`\n",
"* Division: `a * 1/b` (See Fermat's little theorem) \n",
"\n",
"![test](images/ec_math1.jpg)"

@jnewbery jnewbery Sep 15, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The first two columns contain ECKey * ECKey. I think they should say ECKey + ECKey

Comment thread Solutions/0.2-Elliptic-Curve-Math-Solutions.ipynb Outdated
Comment thread 0.2-Elliptic-Curve-Math.ipynb Outdated
Comment thread 0.2-Elliptic-Curve-Math.ipynb Outdated
@jnewbery

jnewbery commented Sep 15, 2019

Copy link
Copy Markdown
Contributor

This is looking pretty good. There are a few review comments from me and @bitschmidty .

@bitschmidty - James is busy for the next couple of days. Are you able to implement all of the review comments in your own commit, and then I'll add it to this PR and merge?

@jnewbery jnewbery mentioned this pull request Sep 16, 2019
Closed
@jnewbery

jnewbery commented Sep 16, 2019

Copy link
Copy Markdown
Contributor

Merging now. The required changes to the images is tracked in #24.

@jnewbery
jnewbery merged commit 076491d into bitcoinops:master Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jnewbery jnewbery jnewbery left review comments
@bitschmidty bitschmidty bitschmidty left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jachiang @jnewbery @bitschmidty