Page MenuHomePhabricator

[secp256k1] scratch: add stack frame support
ClosedPublic

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

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

deadalnix created this revision.Tue, Jan 14, 01:20
Herald added a reviewer: Restricted Project. · View Herald TranscriptTue, Jan 14, 01:20
Fabien requested changes to this revision.Wed, Jan 15, 11:21
Fabien added a subscriber: Fabien.
Fabien added inline comments.
src/secp256k1/src/scratch_impl.h
48 ↗(On Diff #15425)

Should be <= since scratch->frame is a counter and not an index ?

src/secp256k1/src/tests.c
275 ↗(On Diff #15425)

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.Wed, Jan 15, 11:21
deadalnix added inline comments.Wed, Jan 15, 13:40
src/secp256k1/src/scratch_impl.h
48 ↗(On Diff #15425)

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 ↗(On Diff #15425)

Even though this isn't what's in the PR, this seems to be the intended test.

deadalnix updated this revision to Diff 15480.Wed, Jan 15, 13:43

Update erroneous test case

Fabien added inline comments.Wed, Jan 15, 13:45
src/secp256k1/src/scratch_impl.h
48 ↗(On Diff #15425)

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.

deadalnix added inline comments.Wed, Jan 15, 13:49
src/secp256k1/src/scratch_impl.h
48 ↗(On Diff #15425)

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.Wed, Jan 15, 13:51
This revision is now accepted and ready to land.Wed, Jan 15, 13:51
This revision was automatically updated to reflect the committed changes.