Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Schnorr (Incremental) Half Aggregation #261

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

b-wagn
Copy link
Contributor

@b-wagn b-wagn commented Jul 30, 2023

Revisited PR #130 by @jonasnick.
I am happy to hear your thoughts.

Summary of changes compared to #130:

  • Address comments from @rustyrussell
  • Use tagged hash
  • Compute hashes with common prefix by copying midstate
  • Allow Incremental Aggregation and make code consistent with the draft spec

Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

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

Cool, thanks @b-wagn.

Can you remove the added vscode stuff from the commits? This would allow to getting rid of the two "remove vscode settings" commits.

In order to verify that this matches the spec, it would help to add the preliminary test vectors (they don't seem to cover incremental aggregation yet).

@b-wagn b-wagn force-pushed the schnorr-half-agg branch from c2cab8e to 0495479 Compare July 31, 2023 10:22
Copy link
Collaborator

@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.

Here are some comments but I haven't had a closer look yet.

This also needs trivial rebase on master as we made quite a lot of changes there (synced with upstream secp256k1).

include/secp256k1_schnorrsig.h Outdated Show resolved Hide resolved
include/secp256k1_schnorrsig.h Outdated Show resolved Hide resolved
src/modules/schnorrsig/main_impl.h Outdated Show resolved Hide resolved
src/modules/schnorrsig/main_impl.h Outdated Show resolved Hide resolved
src/modules/schnorrsig/main_impl.h Outdated Show resolved Hide resolved
@b-wagn
Copy link
Contributor Author

b-wagn commented Aug 2, 2023

Thanks for your feedback.
I will provide an updated version asap.

@b-wagn b-wagn force-pushed the schnorr-half-agg branch 3 times, most recently from 6045c3d to e49b9e3 Compare August 6, 2023 11:27
Copy link
Collaborator

@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.

This is very clean already, I honestly appreciate the quality of the code.

A somewhat annoying point is that this should probably moved to a separate module. If you want, you can give it a try, but this will involve modifying the build system, too. (Just the autotools build system with configure.ac and Makefile.am. You can ignore the CMake, we don't support it currently in this repo, it's just there because it's in upstream secp256k1 and we would like to keep the diff small.) Happy to help if you want to give it a try; otherwise Jonas or I could just take of it.

.gitignore Outdated Show resolved Hide resolved
ARG_CHECK(new_sigs64 != NULL);

/* Check that aggsig_size is large enough, i.e. aggsig_size >= 32*(n+1) */
n = n_before + n_new;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should return 0 also when the addition here overflows. (I mean, you could say then anyway, something is wrong somewhere else, because the user would think they pass an array of size larger than size_t...) I just think it's more obviously correct (and thus easier to review with the overflow check.)

On a second thought, I wonder whether this should be an ARG_CHECK instead of return 0. Not sure, we'll need to think about it.

src/modules/schnorrsig/main_impl.h Outdated Show resolved Hide resolved
src/modules/schnorrsig/main_impl.h Outdated Show resolved Hide resolved
src/modules/schnorrsig/main_impl.h Outdated Show resolved Hide resolved
src/modules/schnorrsig/main_impl.h Outdated Show resolved Hide resolved
src/modules/schnorrsig/main_impl.h Outdated Show resolved Hide resolved
src/modules/schnorrsig/tests_impl.h Outdated Show resolved Hide resolved
include/secp256k1_schnorrsig.h Outdated Show resolved Hide resolved
Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

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

Here's a (maybe somewhat outdated) template for creating an independent module: 048f9f8

src/modules/schnorrsig/main_impl.h Outdated Show resolved Hide resolved
src/modules/schnorrsig/main_impl.h Outdated Show resolved Hide resolved
include/secp256k1_schnorrsig.h Outdated Show resolved Hide resolved
@jonasnick
Copy link
Contributor

We've just merged #270 which means that this PR can be rebased to fix CI.

configure.ac Outdated
Comment on lines 497 to 511
if test x"$enable_module_schnorrsig_halfagg" = x"yes"; then
SECP_CONFIG_DEFINES="$SECP_CONFIG_DEFINES -DENABLE_MODULE_SCHNORRSIG_HALFAGG=1"
enable_module_schnorrsig=yes
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

You must do this before testing for schnorrsig because the setting enable_module_schnorrsig=yes happens after checking for enable_module_schnorrsig=yes. There's a comment above mentioning this (sorry, hard to find):

# Besides testing whether modules are enabled, the following code also enables
# module dependencies. The order of the tests matters: the dependency must be
# tested first.

Copy link
Collaborator

@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.

@b-wagn I think the main thing to do here from your side is to organize this PR in proper commits. Normally, we would probably prefer small atomic commits (one logical change at a time) but, as this adds an entirely new module instead of changing existing code, all the changes could just be squashed into a single commit. Or you could have one commit for the code, one commit for the tests, and one commit for the build system + CI or something like this. Please don't hesitate to force-push to your branch.

I think then we can do a final review, and I don't except further significant changes then. If you ask me), it's ready for merge then as an experimental module.

We should perhaps add a version number (or some revision identifier) to the BIP draft, as long as the BIP is unstable. We could then say here which version the current code implements. Since it's an experimental module, we don't guarantee API stability, so it's not a big deal if we change the BIP in an incompatible way and need to update the code accordingly.

@@ -0,0 +1,3 @@
include_HEADERS += include/secp256k1_schnorrsig_halfagg.h
noinst_HEADERS += src/modules/schnorrsig_halfagg/main_impl.h
noinst_HEADERS += src/modules/schnorrsig_halfagg/tests_impl.h
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: file should end with a line break

@real-or-random
Copy link
Collaborator

We should perhaps add a version number (or some revision identifier) to the BIP draft, as long as the BIP is unstable. We could then say here which version the current code implements. Since it's an experimental module, we don't guarantee API stability, so it's not a big deal if we change the BIP in an incompatible way and need to update the code accordingly.

Okay, while breaking changes are possible, two things we should think about are the top two items in BlockstreamResearch/cross-input-aggregation#11... Ideally, we would reach a decision on these before we merge this PR, but on the other hand, I don't think these considerations should really hold off this PR. It's already large enough, and changes can also be done in further PRs.

@b-wagn b-wagn force-pushed the schnorr-half-agg branch 2 times, most recently from ee54b1f to 959b070 Compare November 26, 2023 15:47
@b-wagn
Copy link
Contributor Author

b-wagn commented Nov 27, 2023

We should perhaps add a version number (or some revision identifier) to the BIP draft, as long as the BIP is unstable. We could then say here which version the current code implements. Since it's an experimental module, we don't guarantee API stability, so it's not a big deal if we change the BIP in an incompatible way and need to update the code accordingly.

Okay, while breaking changes are possible, two things we should think about are the top two items in BlockstreamResearch/cross-input-aggregation#11... Ideally, we would reach a decision on these before we merge this PR, but on the other hand, I don't think these considerations should really hold off this PR. It's already large enough, and changes can also be done in further PRs.

Either option is fine. I would probably prefer first merging this and then opening a new PR with the z0 = 1 option and properly adjusted tests and so on.

@b-wagn b-wagn marked this pull request as ready for review November 27, 2023 15:06
Copy link
Collaborator

@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.

I still need to look at the tests and the build system/CI stuff again.

include/secp256k1_schnorrsig_halfagg.h Outdated Show resolved Hide resolved
include/secp256k1_schnorrsig_halfagg.h Outdated Show resolved Hide resolved
src/modules/schnorrsig_halfagg/main_impl.h Outdated Show resolved Hide resolved
src/modules/schnorrsig_halfagg/main_impl.h Outdated Show resolved Hide resolved
src/modules/schnorrsig_halfagg/main_impl.h Outdated Show resolved Hide resolved
src/modules/schnorrsig_halfagg/main_impl.h Outdated Show resolved Hide resolved
src/modules/schnorrsig_halfagg/main_impl.h Outdated Show resolved Hide resolved
include/secp256k1_schnorrsig_halfagg.h Show resolved Hide resolved
include/secp256k1_schnorrsig_halfagg.h Outdated Show resolved Hide resolved
include/secp256k1_schnorrsig_halfagg.h Outdated Show resolved Hide resolved
@b-wagn
Copy link
Contributor Author

b-wagn commented Jan 5, 2024

Thank you for your comments @real-or-random.
I addressed them, and I will organize the PR into one commit as soon as your review is completed.

@real-or-random
Copy link
Collaborator

When reviewing the tests, I had a few comments in mind, and then I just started addressing them on my own because I thought that's more efficient in the end. But then this ended up in a larger rewrite. :D (Also one reason why the diff seems so large is that I made the comments a bit more consistent...)

I pushed my suggestions to https://github.com/real-or-random/secp256k1-zkp/tree/schnorr-half-agg. Please don't hesitate to cherry-pick the commit, but please also double-check that I didn't screw up anything...

@b-wagn
Copy link
Contributor Author

b-wagn commented Jan 11, 2024

Thank you @real-or-random. I went through your changes and they make sense to me, so I included them.

@real-or-random
Copy link
Collaborator

Changes look good!

This needs rebase again due to changes in configure.ac (hopefully the last time!), but I just noticed that we'd like to do another change to configure.ac, so let us do this first to avoid that you'll need to rebase here twice...

@real-or-random
Copy link
Collaborator

Changes look good!

This needs rebase again due to changes in configure.ac (hopefully the last time!), but I just noticed that we'd like to do another change to configure.ac, so let us do this first to avoid that you'll need to rebase here twice...

#290 has been merged, so this is ready for rebase. (#290 adds a comment that explains in which order modules should appear in configure.ac and elsewhere.)

@real-or-random
Copy link
Collaborator

Thanks, the NULL changes look good

ACK This is read to merge from my side, mod the following two nits!

I think it will be consistent to do the same then in inc_aggregate (where the first two lines must be moved below the line where we compute n, of course ^^):

    ARG_CHECK(all_pubkeys != NULL || n == 0);
    ARG_CHECK(all_msgs32 != NULL || n== 0);
    ARG_CHECK(new_sigs64 != NULL || n_new == 0);

#290 adds a comment that explains in which order modules should appear in configure.ac and elsewhere.

Have you seen these comments? They're here: https://github.com/BlockstreamResearch/secp256k1-zkp/blob/master/configure.ac#L612-L640

This would mean that the schnorrsig_halfagg module should appear immediately after (or before, if the order is reversed) the bppp whenever we list modules. AFAICT this affects configure.ac (multiple listings), Makefile.am, tests.c, ci.yml, and ci.sh in this PR. It may seem a bit pedantic, but I hope that sticking to that convention will make diffs to upstream libsecp256k1 more manageable in the future.

Copy link
Collaborator

@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.

ACK e1d15fd

Can you squash this into a single commit? Then we can merge it.

Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

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

Concept ACK

I did a few sanity checks and read the include file again. Looks all good. I have a few nits that can be cleaned up in a separate PR. Your detailed and helpful comments in the main code and tests is very much appreciated.

Nits:

  • we may want to add a header to the include file that links to the BIP
  • there's still a mention of aggsig_size
  • we may want to move aggregate before inc_aggregate
  • we should mention expected size of input aggsig array in_aggregate
  • "Should be aggsig_len = 32*(n+1)" -> "Must be"

EDIT: to be clear, I think with @real-or-random's ACK this is ready for merge.

Copy link
Collaborator

@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.

ACK 3a9b1d4

@real-or-random real-or-random merged commit a7907b1 into BlockstreamResearch:master Mar 5, 2024
107 checks passed
@fjahr
Copy link

fjahr commented Jun 28, 2024

@b-wagn Are you interested in opening a PR for this in secp256k1 any time soon? If not, I will give this a try myself because I want to experiment with it in core.

@b-wagn
Copy link
Contributor Author

b-wagn commented Jun 30, 2024

Currently not planning to do so. Feel free to go ahead 👍

@Loopite
Copy link

Loopite commented Aug 9, 2024

Hello there,

I'd like to propose implementing full-aggregated signatures in the code. I came across this information on the Blockstream Research website: Schnorr Signatures for secp256k1. In terms of storage, aggregated signatures are just 64 bytes (R, S), regardless of the number of inputs in transactions, versus 32(n + 1) bytes for [R1, R2, ... Rn], S, where n is the number of public keys. While this is a "good" space-saving measure, it is not maximally efficient.

In fact, when a full-aggregated signature is generated, you can easily verify it by computing the aggregated public key (assuming msg32 is the same for all signatures in the transaction). Schnorr signatures possess this unusual property, and not fully utilizing it might be counterproductive and less effective.

In terms of security, if inputs are unlocked with P2PKH, an attacker would need to "crack" the aggregated public key by finding the private aggregated key. Although this is a tedious task, it would have to be done before the transaction is included in a block, making it MUCH harder. If inputs are unlocked with P2PK and if they can be unlocked back, this attack could be performed before the funds are moved. Therefore, I recommend using full aggregation with P2PKH or other payment methods that hash the public key.

That's why I suggest creating a schnorr_full_aggregate module.

@jonasnick
Copy link
Contributor

Hi @Loopite,

You are correct that "full" aggregation is more efficient. However, one of its major downsides is that it requires an interactive protocol between the signers. There are applications where half aggregation is better suited, while others benefit from "full" aggregation. You may want to check out @fjahr's https://cisaresearch.org/.

I don't think there would be any opposition to a schnorr_full_aggregate module that is based on a reasonable specification/BIP, but that doesn't exist yet.

@Loopite
Copy link

Loopite commented Aug 15, 2024

Hello, thanks for your response.

I wasn’t aware of this useful website before. I’ve noticed that there aren’t many academic papers available on this website. So you’re right that half-signatures are easier to implement.

However, this page mentions that “the formation of a full-agg signature requires an interactive process.” While it can still be manageable with sample wallets, it becomes more challenging when dealing with multiple wallets, as all signers need to be online simultaneously. Nevertheless, it could still be interesting to try implementing full-agg.

I’ll get back to you for further input.

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.

5 participants