Paths

Table of Contentst

Differential D4934

[secp256k1] scratch: add stack frame support
ClosedPublic
Actions

Authored by deadalnix on Jan 14 2020, 01:20.

Tags

None

Subscribers

Details

Reviewers

Group Reviewers

Restricted Project

Commits

rSTAGING6d8c162de5fb: [secp256k1] scratch: add stack frame support
rABC6d8c162de5fb: [secp256k1] scratch: add stack frame support

Summary

This is a backport of libsecp256k1's PR523

Test Plan

ninja check-secp256k1

Diff Detail

Repository

rABC Bitcoin ABC

Branch

secpstackframe

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 8913
Build 15800: Default Diff Build & Tests
Build 15799: arc lint + arc unit

Event Timeline

deadalnix created this revision.Jan 14 2020, 01:20

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 14 2020, 01:20

Build Bitcoin-ABC / Diffs / Diff Testing started.

https://github.com/bitcoin-core/secp256k1/pull/523/files

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8913: Diff 15425.Jan 14 2020, 01:39

Fabien requested changes to this revision.Jan 15 2020, 11:21

Fabien added a subscriber: Fabien.

Fabien added inline comments.

src/secp256k1/src/scratch_impl.h
48	Should be `<=` since `scratch->frame` is a counter and not an index ?
src/secp256k1/src/tests.c
275	`secp256k1_scratch_allocate_frame` doesn't take a boolean argument, this looks like a typo for `CHECK(secp256k1_scratch_allocate_frame(scratch, 500, 1) == 1);`

This revision now requires changes to proceed.Jan 15 2020, 11:21

deadalnix added inline comments.Jan 15 2020, 13:40

src/secp256k1/src/scratch_impl.h
48	This is as per the source material. This is correct if the frames are counted from 0; which they most likely are.
src/secp256k1/src/tests.c
275	Even though this isn't what's in the PR, this seems to be the intended test.

Update erroneous test case

Build Bitcoin-ABC / Diffs / Diff Testing started.

Fabien added inline comments.Jan 15 2020, 13:45

src/secp256k1/src/scratch_impl.h
48	This would be inconsistent with, for example, then assertion in `secp256k1_scratch_destroy` above: `VERIFY_CHECK(scratch->frame == 0);`. If the frames are counted from 0, then there might still be an allocated frame when the scratch is destroyed, causing a memory leak.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8956: Diff 15480.Jan 15 2020, 13:48

deadalnix added inline comments.Jan 15 2020, 13:49

src/secp256k1/src/scratch_impl.h
48	You are correct. This is counting, but the code is correct anyways, because if we are going to allocate a new frame, then we want the frame count to be bellow the max, that that at the end of the process we reach at most the max number of frames.

Fabien accepted this revision.Jan 15 2020, 13:51

This revision is now accepted and ready to land.Jan 15 2020, 13:51

Closed by commit rABC6d8c162de5fb: [secp256k1] scratch: add stack frame support (authored by deadalnix). · Explain WhyJan 15 2020, 13:51

This revision was automatically updated to reflect the committed changes.

deadalnix added a commit: rABC6d8c162de5fb: [secp256k1] scratch: add stack frame support.

deadalnix added a commit: rSTAGING6d8c162de5fb: [secp256k1] scratch: add stack frame support.Apr 7 2020, 22:26

Revision Contents
Changeset List

Path

Size

src/

secp256k1/

include/

4 lines

src/

2 lines

14 lines

26 lines

61 lines

6 lines

53 lines

tests_exhaustive.c

2 lines

Diff 15425

src/secp256k1/include/secp256k1.h

Loading...

src/secp256k1/src/bench_ecmult.c

Loading...

src/secp256k1/src/ecmult_impl.h

Loading...

src/secp256k1/src/scratch.h

Loading...

src/secp256k1/src/scratch_impl.h

Loading...

src/secp256k1/src/secp256k1.c

Loading...

src/secp256k1/src/tests.c

Loading...

src/secp256k1/src/tests_exhaustive.c

Loading...