feat: Forward ports by wwared · Pull Request #49 · argumentcomputer/sphinx

wwared · 2024-06-18T12:56:28Z

This contributes to #38

This ports the following upstream PRs:

chore: fixes succinctlabs/sp1#821 (succinctlabs/sp1@4f3bd7b)

This PR causes a ~20-25% regression to prove_core (+35s for test_prove_epoch_change). See #49 (comment)

This PR reverts some of the upstream changes in 3c9df43, possibly the reason why testnet-v1.0.6 was pulled from upstream. There are follow-up fixes in v1.0.7 and upcoming v1.0.8 dealing with these constraints, and that commit can be reverted once the memory chip constraints are properly updated with the fixes.

Issue #60 was created for tracking a possible minor perf improvement.

Note

This brings us closer to testnet-v1.0.6, which has been pulled from upstream's tags. We are currently slightly ahead of testnet-v1.0.5

Co-authored-by: Ratan Kaliani <ratankaliani@berkeley.edu> Co-authored-by: Kevin Jue <kjue235@gmail.com> Co-authored-by: Eugene Rabinovich <eugene@succinct.xyz> chore: state_mem validity (#871) chore: constraint selectors when is_real zero (#873) chore: circuit poseidon2 babybear (#870) Co-authored-by: John Guibas <jtguibas@gmail.com> fix: nonce in ed decompress (#874) fix: unit tests to test nonces (#875) chore: increase byte lookup channes (#876) chore: update plonk artifacts (#877)

This commit adds the `nonce` column and respective constraints to all chips that didn't yet have it, as well as other minor misc changes.

The MemoryProgram chip was modified to only be included in the first shard. However, the rust out-of-circuit verifier was not updated to account for this since it still expects the chip to be present in every shard, and the in-circuit recursive verifier was updated with constraints that are only valid if the core proof has a single shard. This commit temporarily comments out these additional checks, but this should be reverted when the verifier and Memory chip-related fixes are integrated. This is related to the batch of PRs for issue #38 It might be necessary to revert commit 6aea5dc after the proper fixes are incorporated.

* fix: Update `sha2` commit * fix: Update `ed25519-consensus` * Update `tiny-keccak` * Update `curve25519-dalek`

storojs72

I don't have enough expertise to review this as it should be. Hope you will share your experience at one of our programming sessions

huitseeker

From a performance standpoint, the regression makes this hard to integrate.
From a security standpoint, can we avoid porting the last few commits of succinctlabs/sp1#821 ? That is, can succinctlabs/sp1#853 and its nonce logic (which costs us 20% performance in some stages) be left aside?
/cc @adr1anh

wwared · 2024-06-20T05:35:17Z

@huitseeker, @adr1anh: I've pushed a branch forward_ports_38_reverts that attempts to revert some specific changes one by one.

Context: Comparing only the prove_core time since that's where the performance regression is. Running LC test test_prove_epoch_change, with SHARD_SIZE=4194304 SHARD_BATCH_SIZE=0.

plonk (f1cbdc2): 89.803454692s
forward_ports_38 (a0aa59f): 126.396090489s
forward_ports_38_reverts (83ae16f): 99.831796899s

There is about ~35s of overhead in this PR. Here's a rough breakdown of the performance gains with each specific revert in sequence:

No reverts (a0aa59f): 126.396090489s
Revert nonces (2c2d61b): 121.316983633s (-5s)
Revert range check columns (6e1f306): 117.123038039s (-4s)
Revert NUM_BYTE_LOOKUP_CHANNELS (83ae16f): 99.831796899s (-17s)

Given these results, it doesn't look like the nonce change by itself is the most costly part of the prove_core step, so it's not as simple as reverting that single change to gain the performance back. In fact surprisingly, the performance impact of the nonces and the range check columns combined is not as big as the NUM_BYTE_LOOKUP_CHANNELS change.

There are still other changes in the PR responsible for about ~10s of the overhead that's unaccounted for, but reverting these three changes seem to account for most of the lost performance. However since we don't have too much overhead to spare to meet our performance goals, reverting these might not be enough and requires some more investigation.

huitseeker

After discussion with @adr1anh clarifying the need for the nonce fixes, and @wwared 's helpful information on the performance cost breakdown, it's clear that it's a better approach to consider the overall set of security fixes and find the performance elsewhere.

This PR looks excellent (aside from the poor performance, which isn't exactly a fault in this case), so let's merge it in the plonk feature branch and iterate there!

adr1anh

Crypto looks good

wwared · 2024-06-18T12:56:28Z

This contributes to #38

This ports the following upstream PRs:

chore: fixes succinctlabs/sp1#821 (succinctlabs/sp1@4f3bd7b)

This PR causes a ~20-25% regression to prove_core (+35s for test_prove_epoch_change). See #49 (comment)

This PR reverts some of the upstream changes in 3c9df43, possibly the reason why testnet-v1.0.6 was pulled from upstream. There are follow-up fixes in v1.0.7 and upcoming v1.0.8 dealing with these constraints, and that commit can be reverted once the memory chip constraints are properly updated with the fixes.

Issue #60 was created for tracking a possible minor perf improvement.

Note

This brings us closer to testnet-v1.0.6, which has been pulled from upstream's tags. We are currently slightly ahead of testnet-v1.0.5

Co-authored-by: Ratan Kaliani <ratankaliani@berkeley.edu> Co-authored-by: Kevin Jue <kjue235@gmail.com> Co-authored-by: Eugene Rabinovich <eugene@succinct.xyz> chore: state_mem validity (#871) chore: constraint selectors when is_real zero (#873) chore: circuit poseidon2 babybear (#870) Co-authored-by: John Guibas <jtguibas@gmail.com> fix: nonce in ed decompress (#874) fix: unit tests to test nonces (#875) chore: increase byte lookup channes (#876) chore: update plonk artifacts (#877)

This commit adds the `nonce` column and respective constraints to all chips that didn't yet have it, as well as other minor misc changes.

The MemoryProgram chip was modified to only be included in the first shard. However, the rust out-of-circuit verifier was not updated to account for this since it still expects the chip to be present in every shard, and the in-circuit recursive verifier was updated with constraints that are only valid if the core proof has a single shard. This commit temporarily comments out these additional checks, but this should be reverted when the verifier and Memory chip-related fixes are integrated. This is related to the batch of PRs for issue #38 It might be necessary to revert commit 6aea5dc after the proper fixes are incorporated.

* fix: Update `sha2` commit * fix: Update `ed25519-consensus` * Update `tiny-keccak` * Update `curve25519-dalek`

storojs72

I don't have enough expertise to review this as it should be. Hope you will share your experience at one of our programming sessions

huitseeker

From a performance standpoint, the regression makes this hard to integrate.
From a security standpoint, can we avoid porting the last few commits of succinctlabs/sp1#821 ? That is, can succinctlabs/sp1#853 and its nonce logic (which costs us 20% performance in some stages) be left aside?
/cc @adr1anh

wwared · 2024-06-20T05:35:17Z

@huitseeker, @adr1anh: I've pushed a branch forward_ports_38_reverts that attempts to revert some specific changes one by one.

Context: Comparing only the prove_core time since that's where the performance regression is. Running LC test test_prove_epoch_change, with SHARD_SIZE=4194304 SHARD_BATCH_SIZE=0.

plonk (f1cbdc2): 89.803454692s
forward_ports_38 (a0aa59f): 126.396090489s
forward_ports_38_reverts (83ae16f): 99.831796899s

There is about ~35s of overhead in this PR. Here's a rough breakdown of the performance gains with each specific revert in sequence:

No reverts (a0aa59f): 126.396090489s
Revert nonces (2c2d61b): 121.316983633s (-5s)
Revert range check columns (6e1f306): 117.123038039s (-4s)
Revert NUM_BYTE_LOOKUP_CHANNELS (83ae16f): 99.831796899s (-17s)

Given these results, it doesn't look like the nonce change by itself is the most costly part of the prove_core step, so it's not as simple as reverting that single change to gain the performance back. In fact surprisingly, the performance impact of the nonces and the range check columns combined is not as big as the NUM_BYTE_LOOKUP_CHANNELS change.

There are still other changes in the PR responsible for about ~10s of the overhead that's unaccounted for, but reverting these three changes seem to account for most of the lost performance. However since we don't have too much overhead to spare to meet our performance goals, reverting these might not be enough and requires some more investigation.

huitseeker

After discussion with @adr1anh clarifying the need for the nonce fixes, and @wwared 's helpful information on the performance cost breakdown, it's clear that it's a better approach to consider the overall set of security fixes and find the performance elsewhere.

This PR looks excellent (aside from the poor performance, which isn't exactly a fault in this case), so let's merge it in the plonk feature branch and iterate there!

adr1anh

Crypto looks good

wwared · 2024-06-18T12:56:28Z

This contributes to #38

This ports the following upstream PRs:

chore: fixes succinctlabs/sp1#821 (succinctlabs/sp1@4f3bd7b)

This PR causes a ~20-25% regression to prove_core (+35s for test_prove_epoch_change). See #49 (comment)

This PR reverts some of the upstream changes in 3c9df43, possibly the reason why testnet-v1.0.6 was pulled from upstream. There are follow-up fixes in v1.0.7 and upcoming v1.0.8 dealing with these constraints, and that commit can be reverted once the memory chip constraints are properly updated with the fixes.

Issue #60 was created for tracking a possible minor perf improvement.

Note

This brings us closer to testnet-v1.0.6, which has been pulled from upstream's tags. We are currently slightly ahead of testnet-v1.0.5

Co-authored-by: Ratan Kaliani <ratankaliani@berkeley.edu> Co-authored-by: Kevin Jue <kjue235@gmail.com> Co-authored-by: Eugene Rabinovich <eugene@succinct.xyz> chore: state_mem validity (#871) chore: constraint selectors when is_real zero (#873) chore: circuit poseidon2 babybear (#870) Co-authored-by: John Guibas <jtguibas@gmail.com> fix: nonce in ed decompress (#874) fix: unit tests to test nonces (#875) chore: increase byte lookup channes (#876) chore: update plonk artifacts (#877)

This commit adds the `nonce` column and respective constraints to all chips that didn't yet have it, as well as other minor misc changes.

The MemoryProgram chip was modified to only be included in the first shard. However, the rust out-of-circuit verifier was not updated to account for this since it still expects the chip to be present in every shard, and the in-circuit recursive verifier was updated with constraints that are only valid if the core proof has a single shard. This commit temporarily comments out these additional checks, but this should be reverted when the verifier and Memory chip-related fixes are integrated. This is related to the batch of PRs for issue #38 It might be necessary to revert commit 6aea5dc after the proper fixes are incorporated.

* fix: Update `sha2` commit * fix: Update `ed25519-consensus` * Update `tiny-keccak` * Update `curve25519-dalek`

storojs72

I don't have enough expertise to review this as it should be. Hope you will share your experience at one of our programming sessions

huitseeker

From a performance standpoint, the regression makes this hard to integrate.
From a security standpoint, can we avoid porting the last few commits of succinctlabs/sp1#821 ? That is, can succinctlabs/sp1#853 and its nonce logic (which costs us 20% performance in some stages) be left aside?
/cc @adr1anh

wwared · 2024-06-20T05:35:17Z

@huitseeker, @adr1anh: I've pushed a branch forward_ports_38_reverts that attempts to revert some specific changes one by one.

Context: Comparing only the prove_core time since that's where the performance regression is. Running LC test test_prove_epoch_change, with SHARD_SIZE=4194304 SHARD_BATCH_SIZE=0.

plonk (f1cbdc2): 89.803454692s
forward_ports_38 (a0aa59f): 126.396090489s
forward_ports_38_reverts (83ae16f): 99.831796899s

There is about ~35s of overhead in this PR. Here's a rough breakdown of the performance gains with each specific revert in sequence:

No reverts (a0aa59f): 126.396090489s
Revert nonces (2c2d61b): 121.316983633s (-5s)
Revert range check columns (6e1f306): 117.123038039s (-4s)
Revert NUM_BYTE_LOOKUP_CHANNELS (83ae16f): 99.831796899s (-17s)

Given these results, it doesn't look like the nonce change by itself is the most costly part of the prove_core step, so it's not as simple as reverting that single change to gain the performance back. In fact surprisingly, the performance impact of the nonces and the range check columns combined is not as big as the NUM_BYTE_LOOKUP_CHANNELS change.

There are still other changes in the PR responsible for about ~10s of the overhead that's unaccounted for, but reverting these three changes seem to account for most of the lost performance. However since we don't have too much overhead to spare to meet our performance goals, reverting these might not be enough and requires some more investigation.

huitseeker

After discussion with @adr1anh clarifying the need for the nonce fixes, and @wwared 's helpful information on the performance cost breakdown, it's clear that it's a better approach to consider the overall set of security fixes and find the performance elsewhere.

This PR looks excellent (aside from the poor performance, which isn't exactly a fault in this case), so let's merge it in the plonk feature branch and iterate there!

adr1anh

Crypto looks good

wwared · 2024-06-18T12:56:28Z

This contributes to #38

This ports the following upstream PRs:

chore: fixes succinctlabs/sp1#821 (succinctlabs/sp1@4f3bd7b)

This PR causes a ~20-25% regression to prove_core (+35s for test_prove_epoch_change). See #49 (comment)

This PR reverts some of the upstream changes in 3c9df43, possibly the reason why testnet-v1.0.6 was pulled from upstream. There are follow-up fixes in v1.0.7 and upcoming v1.0.8 dealing with these constraints, and that commit can be reverted once the memory chip constraints are properly updated with the fixes.

Issue #60 was created for tracking a possible minor perf improvement.

Note

This brings us closer to testnet-v1.0.6, which has been pulled from upstream's tags. We are currently slightly ahead of testnet-v1.0.5

Co-authored-by: Ratan Kaliani <ratankaliani@berkeley.edu> Co-authored-by: Kevin Jue <kjue235@gmail.com> Co-authored-by: Eugene Rabinovich <eugene@succinct.xyz> chore: state_mem validity (#871) chore: constraint selectors when is_real zero (#873) chore: circuit poseidon2 babybear (#870) Co-authored-by: John Guibas <jtguibas@gmail.com> fix: nonce in ed decompress (#874) fix: unit tests to test nonces (#875) chore: increase byte lookup channes (#876) chore: update plonk artifacts (#877)

This commit adds the `nonce` column and respective constraints to all chips that didn't yet have it, as well as other minor misc changes.

The MemoryProgram chip was modified to only be included in the first shard. However, the rust out-of-circuit verifier was not updated to account for this since it still expects the chip to be present in every shard, and the in-circuit recursive verifier was updated with constraints that are only valid if the core proof has a single shard. This commit temporarily comments out these additional checks, but this should be reverted when the verifier and Memory chip-related fixes are integrated. This is related to the batch of PRs for issue #38 It might be necessary to revert commit 6aea5dc after the proper fixes are incorporated.

* fix: Update `sha2` commit * fix: Update `ed25519-consensus` * Update `tiny-keccak` * Update `curve25519-dalek`

storojs72

I don't have enough expertise to review this as it should be. Hope you will share your experience at one of our programming sessions

huitseeker

From a performance standpoint, the regression makes this hard to integrate.
From a security standpoint, can we avoid porting the last few commits of succinctlabs/sp1#821 ? That is, can succinctlabs/sp1#853 and its nonce logic (which costs us 20% performance in some stages) be left aside?
/cc @adr1anh

wwared · 2024-06-20T05:35:17Z

@huitseeker, @adr1anh: I've pushed a branch forward_ports_38_reverts that attempts to revert some specific changes one by one.

Context: Comparing only the prove_core time since that's where the performance regression is. Running LC test test_prove_epoch_change, with SHARD_SIZE=4194304 SHARD_BATCH_SIZE=0.

plonk (f1cbdc2): 89.803454692s
forward_ports_38 (a0aa59f): 126.396090489s
forward_ports_38_reverts (83ae16f): 99.831796899s

There is about ~35s of overhead in this PR. Here's a rough breakdown of the performance gains with each specific revert in sequence:

No reverts (a0aa59f): 126.396090489s
Revert nonces (2c2d61b): 121.316983633s (-5s)
Revert range check columns (6e1f306): 117.123038039s (-4s)
Revert NUM_BYTE_LOOKUP_CHANNELS (83ae16f): 99.831796899s (-17s)

Given these results, it doesn't look like the nonce change by itself is the most costly part of the prove_core step, so it's not as simple as reverting that single change to gain the performance back. In fact surprisingly, the performance impact of the nonces and the range check columns combined is not as big as the NUM_BYTE_LOOKUP_CHANNELS change.

There are still other changes in the PR responsible for about ~10s of the overhead that's unaccounted for, but reverting these three changes seem to account for most of the lost performance. However since we don't have too much overhead to spare to meet our performance goals, reverting these might not be enough and requires some more investigation.

huitseeker

After discussion with @adr1anh clarifying the need for the nonce fixes, and @wwared 's helpful information on the performance cost breakdown, it's clear that it's a better approach to consider the overall set of security fixes and find the performance elsewhere.

This PR looks excellent (aside from the poor performance, which isn't exactly a fault in this case), so let's merge it in the plonk feature branch and iterate there!

adr1anh

Crypto looks good

wwared · 2024-06-18T12:56:28Z

This contributes to #38

This ports the following upstream PRs:

chore: fixes succinctlabs/sp1#821 (succinctlabs/sp1@4f3bd7b)

This PR causes a ~20-25% regression to prove_core (+35s for test_prove_epoch_change). See #49 (comment)

This PR reverts some of the upstream changes in 3c9df43, possibly the reason why testnet-v1.0.6 was pulled from upstream. There are follow-up fixes in v1.0.7 and upcoming v1.0.8 dealing with these constraints, and that commit can be reverted once the memory chip constraints are properly updated with the fixes.

Issue #60 was created for tracking a possible minor perf improvement.

Note

This brings us closer to testnet-v1.0.6, which has been pulled from upstream's tags. We are currently slightly ahead of testnet-v1.0.5

Co-authored-by: Ratan Kaliani <ratankaliani@berkeley.edu> Co-authored-by: Kevin Jue <kjue235@gmail.com> Co-authored-by: Eugene Rabinovich <eugene@succinct.xyz> chore: state_mem validity (#871) chore: constraint selectors when is_real zero (#873) chore: circuit poseidon2 babybear (#870) Co-authored-by: John Guibas <jtguibas@gmail.com> fix: nonce in ed decompress (#874) fix: unit tests to test nonces (#875) chore: increase byte lookup channes (#876) chore: update plonk artifacts (#877)

This commit adds the `nonce` column and respective constraints to all chips that didn't yet have it, as well as other minor misc changes.

The MemoryProgram chip was modified to only be included in the first shard. However, the rust out-of-circuit verifier was not updated to account for this since it still expects the chip to be present in every shard, and the in-circuit recursive verifier was updated with constraints that are only valid if the core proof has a single shard. This commit temporarily comments out these additional checks, but this should be reverted when the verifier and Memory chip-related fixes are integrated. This is related to the batch of PRs for issue #38 It might be necessary to revert commit 6aea5dc after the proper fixes are incorporated.

* fix: Update `sha2` commit * fix: Update `ed25519-consensus` * Update `tiny-keccak` * Update `curve25519-dalek`

storojs72

I don't have enough expertise to review this as it should be. Hope you will share your experience at one of our programming sessions

huitseeker

From a performance standpoint, the regression makes this hard to integrate.
From a security standpoint, can we avoid porting the last few commits of succinctlabs/sp1#821 ? That is, can succinctlabs/sp1#853 and its nonce logic (which costs us 20% performance in some stages) be left aside?
/cc @adr1anh

wwared · 2024-06-20T05:35:17Z

@huitseeker, @adr1anh: I've pushed a branch forward_ports_38_reverts that attempts to revert some specific changes one by one.

Context: Comparing only the prove_core time since that's where the performance regression is. Running LC test test_prove_epoch_change, with SHARD_SIZE=4194304 SHARD_BATCH_SIZE=0.

plonk (f1cbdc2): 89.803454692s
forward_ports_38 (a0aa59f): 126.396090489s
forward_ports_38_reverts (83ae16f): 99.831796899s

There is about ~35s of overhead in this PR. Here's a rough breakdown of the performance gains with each specific revert in sequence:

No reverts (a0aa59f): 126.396090489s
Revert nonces (2c2d61b): 121.316983633s (-5s)
Revert range check columns (6e1f306): 117.123038039s (-4s)
Revert NUM_BYTE_LOOKUP_CHANNELS (83ae16f): 99.831796899s (-17s)

Given these results, it doesn't look like the nonce change by itself is the most costly part of the prove_core step, so it's not as simple as reverting that single change to gain the performance back. In fact surprisingly, the performance impact of the nonces and the range check columns combined is not as big as the NUM_BYTE_LOOKUP_CHANNELS change.

There are still other changes in the PR responsible for about ~10s of the overhead that's unaccounted for, but reverting these three changes seem to account for most of the lost performance. However since we don't have too much overhead to spare to meet our performance goals, reverting these might not be enough and requires some more investigation.

huitseeker

After discussion with @adr1anh clarifying the need for the nonce fixes, and @wwared 's helpful information on the performance cost breakdown, it's clear that it's a better approach to consider the overall set of security fixes and find the performance elsewhere.

This PR looks excellent (aside from the poor performance, which isn't exactly a fault in this case), so let's merge it in the plonk feature branch and iterate there!

adr1anh

Crypto looks good

wwared · 2024-06-18T12:56:28Z

This contributes to #38

This ports the following upstream PRs:

chore: fixes succinctlabs/sp1#821 (succinctlabs/sp1@4f3bd7b)

This PR causes a ~20-25% regression to prove_core (+35s for test_prove_epoch_change). See #49 (comment)

This PR reverts some of the upstream changes in 3c9df43, possibly the reason why testnet-v1.0.6 was pulled from upstream. There are follow-up fixes in v1.0.7 and upcoming v1.0.8 dealing with these constraints, and that commit can be reverted once the memory chip constraints are properly updated with the fixes.

Issue #60 was created for tracking a possible minor perf improvement.

Note

This brings us closer to testnet-v1.0.6, which has been pulled from upstream's tags. We are currently slightly ahead of testnet-v1.0.5

Co-authored-by: Ratan Kaliani <ratankaliani@berkeley.edu> Co-authored-by: Kevin Jue <kjue235@gmail.com> Co-authored-by: Eugene Rabinovich <eugene@succinct.xyz> chore: state_mem validity (#871) chore: constraint selectors when is_real zero (#873) chore: circuit poseidon2 babybear (#870) Co-authored-by: John Guibas <jtguibas@gmail.com> fix: nonce in ed decompress (#874) fix: unit tests to test nonces (#875) chore: increase byte lookup channes (#876) chore: update plonk artifacts (#877)

This commit adds the `nonce` column and respective constraints to all chips that didn't yet have it, as well as other minor misc changes.

The MemoryProgram chip was modified to only be included in the first shard. However, the rust out-of-circuit verifier was not updated to account for this since it still expects the chip to be present in every shard, and the in-circuit recursive verifier was updated with constraints that are only valid if the core proof has a single shard. This commit temporarily comments out these additional checks, but this should be reverted when the verifier and Memory chip-related fixes are integrated. This is related to the batch of PRs for issue #38 It might be necessary to revert commit 6aea5dc after the proper fixes are incorporated.

* fix: Update `sha2` commit * fix: Update `ed25519-consensus` * Update `tiny-keccak` * Update `curve25519-dalek`

storojs72

I don't have enough expertise to review this as it should be. Hope you will share your experience at one of our programming sessions

huitseeker

From a performance standpoint, the regression makes this hard to integrate.
From a security standpoint, can we avoid porting the last few commits of succinctlabs/sp1#821 ? That is, can succinctlabs/sp1#853 and its nonce logic (which costs us 20% performance in some stages) be left aside?
/cc @adr1anh

wwared · 2024-06-20T05:35:17Z

@huitseeker, @adr1anh: I've pushed a branch forward_ports_38_reverts that attempts to revert some specific changes one by one.

Context: Comparing only the prove_core time since that's where the performance regression is. Running LC test test_prove_epoch_change, with SHARD_SIZE=4194304 SHARD_BATCH_SIZE=0.

plonk (f1cbdc2): 89.803454692s
forward_ports_38 (a0aa59f): 126.396090489s
forward_ports_38_reverts (83ae16f): 99.831796899s

There is about ~35s of overhead in this PR. Here's a rough breakdown of the performance gains with each specific revert in sequence:

No reverts (a0aa59f): 126.396090489s
Revert nonces (2c2d61b): 121.316983633s (-5s)
Revert range check columns (6e1f306): 117.123038039s (-4s)
Revert NUM_BYTE_LOOKUP_CHANNELS (83ae16f): 99.831796899s (-17s)

Given these results, it doesn't look like the nonce change by itself is the most costly part of the prove_core step, so it's not as simple as reverting that single change to gain the performance back. In fact surprisingly, the performance impact of the nonces and the range check columns combined is not as big as the NUM_BYTE_LOOKUP_CHANNELS change.

There are still other changes in the PR responsible for about ~10s of the overhead that's unaccounted for, but reverting these three changes seem to account for most of the lost performance. However since we don't have too much overhead to spare to meet our performance goals, reverting these might not be enough and requires some more investigation.

huitseeker

After discussion with @adr1anh clarifying the need for the nonce fixes, and @wwared 's helpful information on the performance cost breakdown, it's clear that it's a better approach to consider the overall set of security fixes and find the performance elsewhere.

This PR looks excellent (aside from the poor performance, which isn't exactly a fault in this case), so let's merge it in the plonk feature branch and iterate there!

adr1anh

Crypto looks good

wwared · 2024-06-18T12:56:28Z

This contributes to #38

This ports the following upstream PRs:

chore: fixes succinctlabs/sp1#821 (succinctlabs/sp1@4f3bd7b)

This PR causes a ~20-25% regression to prove_core (+35s for test_prove_epoch_change). See #49 (comment)

This PR reverts some of the upstream changes in 3c9df43, possibly the reason why testnet-v1.0.6 was pulled from upstream. There are follow-up fixes in v1.0.7 and upcoming v1.0.8 dealing with these constraints, and that commit can be reverted once the memory chip constraints are properly updated with the fixes.

Issue #60 was created for tracking a possible minor perf improvement.

Note

This brings us closer to testnet-v1.0.6, which has been pulled from upstream's tags. We are currently slightly ahead of testnet-v1.0.5

Co-authored-by: Ratan Kaliani <ratankaliani@berkeley.edu> Co-authored-by: Kevin Jue <kjue235@gmail.com> Co-authored-by: Eugene Rabinovich <eugene@succinct.xyz> chore: state_mem validity (#871) chore: constraint selectors when is_real zero (#873) chore: circuit poseidon2 babybear (#870) Co-authored-by: John Guibas <jtguibas@gmail.com> fix: nonce in ed decompress (#874) fix: unit tests to test nonces (#875) chore: increase byte lookup channes (#876) chore: update plonk artifacts (#877)

This commit adds the `nonce` column and respective constraints to all chips that didn't yet have it, as well as other minor misc changes.

The MemoryProgram chip was modified to only be included in the first shard. However, the rust out-of-circuit verifier was not updated to account for this since it still expects the chip to be present in every shard, and the in-circuit recursive verifier was updated with constraints that are only valid if the core proof has a single shard. This commit temporarily comments out these additional checks, but this should be reverted when the verifier and Memory chip-related fixes are integrated. This is related to the batch of PRs for issue #38 It might be necessary to revert commit 6aea5dc after the proper fixes are incorporated.

* fix: Update `sha2` commit * fix: Update `ed25519-consensus` * Update `tiny-keccak` * Update `curve25519-dalek`

storojs72

I don't have enough expertise to review this as it should be. Hope you will share your experience at one of our programming sessions

huitseeker

From a performance standpoint, the regression makes this hard to integrate.
From a security standpoint, can we avoid porting the last few commits of succinctlabs/sp1#821 ? That is, can succinctlabs/sp1#853 and its nonce logic (which costs us 20% performance in some stages) be left aside?
/cc @adr1anh

wwared · 2024-06-20T05:35:17Z

@huitseeker, @adr1anh: I've pushed a branch forward_ports_38_reverts that attempts to revert some specific changes one by one.

Context: Comparing only the prove_core time since that's where the performance regression is. Running LC test test_prove_epoch_change, with SHARD_SIZE=4194304 SHARD_BATCH_SIZE=0.

plonk (f1cbdc2): 89.803454692s
forward_ports_38 (a0aa59f): 126.396090489s
forward_ports_38_reverts (83ae16f): 99.831796899s

There is about ~35s of overhead in this PR. Here's a rough breakdown of the performance gains with each specific revert in sequence:

No reverts (a0aa59f): 126.396090489s
Revert nonces (2c2d61b): 121.316983633s (-5s)
Revert range check columns (6e1f306): 117.123038039s (-4s)
Revert NUM_BYTE_LOOKUP_CHANNELS (83ae16f): 99.831796899s (-17s)

Given these results, it doesn't look like the nonce change by itself is the most costly part of the prove_core step, so it's not as simple as reverting that single change to gain the performance back. In fact surprisingly, the performance impact of the nonces and the range check columns combined is not as big as the NUM_BYTE_LOOKUP_CHANNELS change.

There are still other changes in the PR responsible for about ~10s of the overhead that's unaccounted for, but reverting these three changes seem to account for most of the lost performance. However since we don't have too much overhead to spare to meet our performance goals, reverting these might not be enough and requires some more investigation.

huitseeker

After discussion with @adr1anh clarifying the need for the nonce fixes, and @wwared 's helpful information on the performance cost breakdown, it's clear that it's a better approach to consider the overall set of security fixes and find the performance elsewhere.

This PR looks excellent (aside from the poor performance, which isn't exactly a fault in this case), so let's merge it in the plonk feature branch and iterate there!

adr1anh

Crypto looks good

wwared · 2024-06-18T12:56:28Z

This contributes to #38

This ports the following upstream PRs:

chore: fixes succinctlabs/sp1#821 (succinctlabs/sp1@4f3bd7b)

This PR causes a ~20-25% regression to prove_core (+35s for test_prove_epoch_change). See #49 (comment)

This PR reverts some of the upstream changes in 3c9df43, possibly the reason why testnet-v1.0.6 was pulled from upstream. There are follow-up fixes in v1.0.7 and upcoming v1.0.8 dealing with these constraints, and that commit can be reverted once the memory chip constraints are properly updated with the fixes.

Issue #60 was created for tracking a possible minor perf improvement.

Note

This brings us closer to testnet-v1.0.6, which has been pulled from upstream's tags. We are currently slightly ahead of testnet-v1.0.5

Co-authored-by: Ratan Kaliani <ratankaliani@berkeley.edu> Co-authored-by: Kevin Jue <kjue235@gmail.com> Co-authored-by: Eugene Rabinovich <eugene@succinct.xyz> chore: state_mem validity (#871) chore: constraint selectors when is_real zero (#873) chore: circuit poseidon2 babybear (#870) Co-authored-by: John Guibas <jtguibas@gmail.com> fix: nonce in ed decompress (#874) fix: unit tests to test nonces (#875) chore: increase byte lookup channes (#876) chore: update plonk artifacts (#877)

This commit adds the `nonce` column and respective constraints to all chips that didn't yet have it, as well as other minor misc changes.

The MemoryProgram chip was modified to only be included in the first shard. However, the rust out-of-circuit verifier was not updated to account for this since it still expects the chip to be present in every shard, and the in-circuit recursive verifier was updated with constraints that are only valid if the core proof has a single shard. This commit temporarily comments out these additional checks, but this should be reverted when the verifier and Memory chip-related fixes are integrated. This is related to the batch of PRs for issue #38 It might be necessary to revert commit 6aea5dc after the proper fixes are incorporated.

* fix: Update `sha2` commit * fix: Update `ed25519-consensus` * Update `tiny-keccak` * Update `curve25519-dalek`

storojs72

I don't have enough expertise to review this as it should be. Hope you will share your experience at one of our programming sessions

huitseeker

From a performance standpoint, the regression makes this hard to integrate.
From a security standpoint, can we avoid porting the last few commits of succinctlabs/sp1#821 ? That is, can succinctlabs/sp1#853 and its nonce logic (which costs us 20% performance in some stages) be left aside?
/cc @adr1anh

wwared · 2024-06-20T05:35:17Z

@huitseeker, @adr1anh: I've pushed a branch forward_ports_38_reverts that attempts to revert some specific changes one by one.

Context: Comparing only the prove_core time since that's where the performance regression is. Running LC test test_prove_epoch_change, with SHARD_SIZE=4194304 SHARD_BATCH_SIZE=0.

plonk (f1cbdc2): 89.803454692s
forward_ports_38 (a0aa59f): 126.396090489s
forward_ports_38_reverts (83ae16f): 99.831796899s

There is about ~35s of overhead in this PR. Here's a rough breakdown of the performance gains with each specific revert in sequence:

No reverts (a0aa59f): 126.396090489s
Revert nonces (2c2d61b): 121.316983633s (-5s)
Revert range check columns (6e1f306): 117.123038039s (-4s)
Revert NUM_BYTE_LOOKUP_CHANNELS (83ae16f): 99.831796899s (-17s)

Given these results, it doesn't look like the nonce change by itself is the most costly part of the prove_core step, so it's not as simple as reverting that single change to gain the performance back. In fact surprisingly, the performance impact of the nonces and the range check columns combined is not as big as the NUM_BYTE_LOOKUP_CHANNELS change.

There are still other changes in the PR responsible for about ~10s of the overhead that's unaccounted for, but reverting these three changes seem to account for most of the lost performance. However since we don't have too much overhead to spare to meet our performance goals, reverting these might not be enough and requires some more investigation.

huitseeker

After discussion with @adr1anh clarifying the need for the nonce fixes, and @wwared 's helpful information on the performance cost breakdown, it's clear that it's a better approach to consider the overall set of security fixes and find the performance elsewhere.

This PR looks excellent (aside from the poor performance, which isn't exactly a fault in this case), so let's merge it in the plonk feature branch and iterate there!

adr1anh

Crypto looks good

wwared changed the title ~~Forward ports 38~~feat: Forward ports 38Jun 18, 2024

wwared force-pushed the forward_ports_38 branch 4 times, most recently from 7faa80c to a97010bCompare June 19, 2024 21:13

wwared requested review from adr1anh, huitseeker and storojs72 June 19, 2024 21:14

wwared marked this pull request as ready for review June 19, 2024 21:14

wwared changed the title ~~feat: Forward ports 38~~feat: Forward portsJun 19, 2024

wwared force-pushed the forward_ports_38 branch from a97010b to 2432647Compare June 19, 2024 21:29

wwaredand others added 4 commits June 19, 2024 18:32

fix: Adapt Sphinx chips to nonce changes
8307315
This commit adds the `nonce` column and respective constraints to all chips that didn't yet have it, as well as other minor misc changes.

chore: clippy and fmt
0977f63

fix: Updatesp1-patchescommits (#56)
ebb0730
* fix: Update `sha2` commit * fix: Update `ed25519-consensus` * Update `tiny-keccak` * Update `curve25519-dalek`

wwared force-pushed the forward_ports_38 branch from 2432647 to ebb0730Compare June 19, 2024 21:32

wwared mentioned this pull request Jun 19, 2024
feat: Solidity verification of light client proofs with Plonk argumentcomputer/zk-light-clients#28
Merged

storojs72 approved these changes Jun 19, 2024
View reviewed changes

wwared mentioned this pull request Jun 19, 2024
feat: Merge plonk into dev#61
Closed

huitseeker reviewed Jun 20, 2024
View reviewed changes

huitseeker approved these changes Jun 20, 2024
View reviewed changes

adr1anh approved these changes Jun 20, 2024
View reviewed changes

wwared merged commit 317414f into plonkJun 20, 2024

wwared deleted the forward_ports_38 branch June 20, 2024 10:37

wwared restored the forward_ports_38 branch June 20, 2024 10:38

storojs72 mentioned this pull request Jun 20, 2024
feat: Incorporate downloading Sphinx parameters using AWS cli #63
Merged