Skip to content

Conversation

@real-or-random
Copy link
Contributor

As discussed in #1126.

For randomization, this has a history. Initially, this threw the illegal callback but then we changed it to be a no-op on non-signing contexts: 6198375 But this was with (non-static) none/verification contexts in mind, not with the static context. If we anyway forbid cloning the static context, you should never a way to randomize a copy of the static context. (You need a copy because the static context itself is not writable. But you cannot obtain a copy except when using memcpy etc.)

@real-or-random
Copy link
Contributor Author

real-or-random commented Dec 7, 2022

force-pushed to clarify a comment and fix a commit message, ready for review

edit: okay, really ready now, I promise.

@sipa
Copy link
Contributor

sipa commented Dec 7, 2022

src/tests.c:7420: test condition failed: !secp256k1_context_is_proper(ctx)

@real-or-random
Copy link
Contributor Author

.... should be fixed. (If you think you can write a single line of code without rerunning the tests...)

@sipa
Copy link
Contributor

sipa commented Dec 7, 2022

utACK 29cac36

@real-or-random
Copy link
Contributor Author

real-or-random commented Dec 8, 2022

I'm starting to think that it would in fact be preferable not to have this in 0.2.0.

The changes here are not crucial or urgent. And they're strictly speaking still breaking changes, and so they'd fit a 0.3.0 or 1.0.0 nicely, where the version bump will indicate that the API has changed.

@sipa
Copy link
Contributor

sipa commented Dec 8, 2022

@real-or-random Sounds good. Making breaking changes (no matter how small) after we've started doing releases is better in reducing API ambiguity.

Are there perhaps still some cleanups from this PR we'd want?

@real-or-random
Copy link
Contributor Author

Are these perhaps still some cleanups from this PR we'd want?

The second commit is certainly a (tiny) fix. We should abort the API function (with return) for the case that a callback returns (callbacks aren't supposed to return but yeah.)

Let me create a PR that doesn't have the breaking API changes and the test changes.

@real-or-random
Copy link
Contributor Author

Let me create a PR that doesn't have the breaking API changes and the test changes.

#1171, marking this one a draft.

@real-or-random real-or-random marked this pull request as draft December 8, 2022 15:36
Copy link
Contributor Author

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

A cleaner way will be to split run_context_tests() into one function for normal contexts (which has its separate local my_ctx) and one for static contexts (which can use the global sttc).

@real-or-random
Copy link
Contributor Author

(force-pushed a rebased version here but please ignore... the plan is to get #1186 merged first and then I need to reapply the interesting commits from here on top of it)

@real-or-random real-or-random force-pushed the 202212-static-ctx-api branch 2 times, most recently from 7c7b2fe to 71caa81 Compare January 17, 2023 11:33
@real-or-random real-or-random marked this pull request as ready for review January 17, 2023 11:33
@real-or-random
Copy link
Contributor Author

real-or-random commented Jan 17, 2023

Ready for review.

I should have done the last commit first, but I want to avoid the effort to reorder unless someone insists. @apoelstra This introduces the macro you proposed. I initially didn't plan to add it in this PR but I got too annoyed by the ecounts. Another PR can use it in the rest of the tests.

@real-or-random
Copy link
Contributor Author

force-pushed to make LSan happy (hopefully)

{ CHECK((expr) == 0); } \
secp256k1_context_set_illegal_callback(ctx, NULL, NULL); \
CHECK(ILLEGAL == 1); \
} while(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

In 0403278:

I'd prefer that ILLEGAL be a local variable and that we used the data pointer to pass it into the callback function, rather than having it be a global.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I had feared this makes the job of the optimizer harder, but maybe that concern is unnecessary (or even wrong).

I'll fix it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It probably will make the optimizer's job harder but I doubt it'll be noticeable, and these are only unit tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@real-or-random real-or-random force-pushed the 202212-static-ctx-api branch 3 times, most recently from abcd6e9 to 7a6cc4d Compare January 18, 2023 13:35
@real-or-random
Copy link
Contributor Author

Ooops, that line ended up in the wrong commit after rebasing. Should be fixed!

Copy link
Contributor

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 6862279

Copy link
Contributor

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK e39d954

@sipa
Copy link
Contributor

sipa commented Jan 19, 2023

utACK e39d954

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.

3 participants