Skip to content

Conversation

real-or-random
Copy link
Collaborator

After some more consideration. I think the original behavior of ignoring overflows in input s values during aggregation was better. It simply removes an error path from the user. (For some background, BIP340 intentionally didn't distinguish between an invalid sig or an unparseable sig. This avoids having two distinct error paths where everyone just needs a single one.)

One could think that this change now could lead to cases where inputs sigs are invalid (due to overflow in s), but the resulting aggsig is valid. But first, such input sigs are computationally hard to create if the hash function is good, and second, we anyway can't hold up the property that invalid input sigs always produce an invalid aggsig (unless we verify inputs upfront), as we now note in the BIP.

Comment on lines +133 to +135
/// Constructs two invalid signatures whose aggregate signature is valid
#[test]
fn test_aggregate_verify_strange() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the rest of the file ist rustfmt'd and this function does not look like it is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I fixed most of the stuff reported by rustfmt, but I don't agree with one suggestion in my code, and it also shows another suggestion in existing code. (Also happy to just accept all its suggestions if we want this.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Imo the point of rustfmt is to avoid having to think about formatting at all. So I have a slight preference for just rustmt'ing everything. The other suggestion in existing code may just be a relic of forgetting to rustfmt.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, let me include this in the next PR, I have some local changes already on top of this PR.

Imo the point of rustfmt is to avoid having to think about formatting at all.

Yep, I think that's the advantage, and that's fine with me.

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.

ACK df69673

@jonasnick jonasnick merged commit 8a9d6fb into BlockstreamResearch:master Jan 17, 2024
code-druidx6y78 added a commit to code-druidx6y78/cross-input-aggregation that referenced this pull request Sep 28, 2025
…ow handling

df696736e26a8f6e03b9c04d1fde0ac7888337b1 halfagg: Note that invalid sigs can have valid aggsig and add test (Tim Ruffing)
64609172f401cf586f0f1335d1343119e0828d05 Revert "halfagg: fail inc_agg if input scalars overflow" (Tim Ruffing)

Pull request description:

ACKs for top commit:
  jonasnick:
    ACK df696736e26a8f6e03b9c04d1fde0ac7888337b1

Tree-SHA512: 84e70bf39533bd48f917f7e692f1422be54c86aae2d854cdd661e473d1a6a2f194e2b43779981c86196633985b98b061ce20c4cb30ba8280e3f3669c71638664
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.

2 participants