Closed
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Contributor
|
@mralj I'm worried about the massive change in Can you walk me through how the lockfile was changed? I want to make sure we're using the exact same deps, except for the ones we explicitly changed, so we are not hit by any other unexpected errors. NethermindEth/discv5@18af0cb#diff-51e4f558fae534656963876761c95b83b6ef5da5103c4adef6768219ed76c2de |
Contributor
Author
Good point!
Let me see if this will work w/o the yarn upgrade, so please don't merge just yet. P.S. |
Contributor
Author
|
@spalladino much saner yarn.lock diff. |
Contributor
Fair point! |
spalladino
approved these changes
Oct 13, 2025
…ontract tutorial - Updated import statements for clarity and consistency, replacing `use aztec::prelude` with `protocol_types` for better context. - Removed the `Prevent double spending` section and the `emit_in_public` function to streamline the tutorial and focus on essential functionalities. - Adjusted the `increment` function to eliminate redundant code while maintaining its core functionality.
PXE is becoming just a lib used by the wallet and for this reason I am dropping the corresponding JSON RPC Server. This leads to a large refactor of basically all the setups that dealt with PXE and wallets. This is a followup work of [this PR](#17163) in which I had this tasks planned: 1. Drop PXE JSON RPC server, 2. have Wallet instantiate PXE instead of having it passed in as an arg to a constructor, 3. revisit the PXE methods exposed on the TestWallet. In this PR I tackled points 1 and 2.
This PR refactors staging network deployments in an attempt to reduce complexity. Co-authored-by: Phil Windle <philip.windle@gmail.com>
Please read [contributing guidelines](CONTRIBUTING.md) and remove this line. For audit-related pull requests, please use the [audit PR template](?expand=1&template=audit.md).
Fixes the issue found by @federicobarbacovi in `secp256k1_ecdsa_mul`. The issue was that the point at infinity flag `_is_infinity` was not being set explicitly when adding base points due to the skew flags. This issue arises when in the following edge case: ### Description of the issue Choose secp256k1 scalars $s_1, u_1, u_2 \in \mathbb{F}_r$ such that 1. the public key is computed as $P = s_1 \cdot G$ for generator $G \in \mathbb{G}$ 2. $u_1 \cdot G + u_2 \cdot P = \mathcal{O}$ where $\mathcal{O}$ is the point at infinity. In the `secp256k1_ecdsa_mul` function, the scalars $u_1$ and $u_2$ are split in endomorphism scalars: $$u_1 = u_{1, \textsf{low}} + \lambda \cdot u_{1, \textsf{high}}, \quad u_2 = u_{2, \textsf{low}} + \lambda \cdot u_{2, \textsf{high}}.$$ Each of the scalars $u_{1, \textsf{low}}, u_{1, \textsf{high}}, u_{2, \textsf{low}}, u_{2, \textsf{high}}$ has a skew bit: let them be $b_1, b_2, b_3, b_4$. We handle the skew bit in the accumulator $A$ as: 1. First compute addition $B = A + G \quad \text{or} \quad B = A - G$ based on the skew being positive or negative 2. Set the accumulator to $B$ if the skew is non-zero, else keep the accumulator unchanged: $A := (1 - b) \cdot A + b \cdot B$ While performing the second step, we conditionally selected only the `x` and `y` coordinates of $A$ or $B$ but didn't fetch the `_is_infinity` flag. This leads to the flag `_is_infinity` set to `false` even when the result of the scalar multiplication is point at infinity. ### Fix The fix is to correctly set the `_is_infinity` flag when performing conditional selection operation. So we just define a new `conditional_select` function on the `biggroup` (and `biggroup_goblin`) class. --------- Co-authored-by: federicobarbacovi <federicobarbacovi@users.noreply.github.com>
Adds a flag to always reexecute block proposals. If set, a validator node will always reexecute, even if not part of the committee, though they will not attest. If the node is not a validator, they will just log the result fo the execution. Note that this does NOT affect p2p propagation, since the reexecution is done after the attestation is propagated, as it happens on a separate handler and not in a p2p-registered validator. To handle reexecutions in a non-validator node, reexecution was moved to a block-proposal-handler class, which is instantiated instead of a validator client in non-validators. This PR also causes validators to reexecute a proposal if they are not in the committee if there is a slash penalty defined for broadcasting invalid block proposals. Since this feature is not yet properly tested, I've disabled the default slash for these offenses for the time being (they were not working at the moment). See A-57 for more info. Fixes A-54
Fixes the issue found by @federicobarbacovi in `secp256k1_ecdsa_mul`. The issue was that while adding the stagger points, we were using incomplete addition formula (via `chain_add_start`, `chain_add`, `chain_add_end` functions). This leads to unsatisfiable circuit for certain sets of scalars $s_1, u_1, u_2 \in \mathbb{F}_r$ such that: 1. the public key is computed as $P = s_1 \cdot G$ for generator $G \in \mathbb{G}$ 2. $u_1 \cdot G + u_2 \cdot P = \mathcal{O}$ where $\mathcal{O}$ is the point at infinity. In particular, when the skew terms of each of the scalars $u_{1, \textsf{low}}, u_{1, \textsf{high}}, u_{2, \textsf{low}}, u_{2, \textsf{high}}$ is 0, we know that the accumulator would reach the point at infinity before handling any skew terms. In this case, that happens when performing point additions due to the stagger terms. We know the stagger offsets of the scalars are set as: | Term | Stagger bits | Point to add | |-------------|--------------|-------------| | $u_{1, \textsf{low}}$ | 2 | $P_3 := 2G$ | |$u_{1, \textsf{high}}$ | 3 | $P_1 := 3λG$ | |$u_{2, \textsf{low}}$ | 0 | — | | $u_{2, \textsf{high}}$ | 1 | $P_2 := λG$ | For the regression test case in this PR, we first add $P_1, P_2$: $A \longleftarrow (A +P_1)$ $A \longleftarrow (A + P_2)$ While adding $P_3$, we reach the point at infinity: $A + P_3 = \mathcal{O}$. To fix this, we simply use complete addition formula for adding each of $P_1, P_2, P_3$. This increases the circuit size for `secp256k1_ecdsa_mul` by 730 gates ($\approx 2$ percent increase in circuit size from 39957 to 40687 gates).
### 🧾 Audit Context Cleanup and audit of the lookup tables used in `biggroup` class. This PR does not change any logic, its mainly structural changes, cleanup and documentation. ### 🛠️ Changes Made - Added a README to explain how lookup tables are being used in biggroup, and a brief summary of how ROM tables work in barretenberg circuits. - Removed unused methods/structs like: `lookup_table_base`, `batch_lookup_table_base`, `create_endo_pair_five_lookup_table`. - Function comments + fix clang warnings + avoid hard-coded values. - No change to circuit, so no VK changes expected. ### ✅ Checklist - [x] Audited all methods of the relevant module/class - [x] Audited the interface of the module/class with other (relevant) components - [x] Documented existing functionality and any changes made (as per Doxygen requirements) - [x] Resolved and/or closed all issues/TODOs pertaining to the audited files - [x] Confirmed and documented any security or other issues found (if applicable) - [x] Verified that tests cover all critical paths (and added tests if necessary) - [ ] Updated audit tracking for the files audited (check the start of each file you audited) ### 📌 Notes for Reviewers NA
While we keep slashing rounds for a long time (lifetime in rounds is currently defined as 100), we only attempt to execute them during the first round they become executable. If for whatever reason they don't get executed, they are just forgotten and never actually triggered. This PR adds a new config `slashExecuteRoundsLookBack` (defaults to 4) with how many execution rounds to look back from the latest executable round to see if there was any round pending execution. Each round is checked in sequence, so setting this value too high can introduce performance issues. Setting this value to zero keeps the same behaviour as we have today. This PR also fixes another issue: we were re-checking if a round was executable based on the isReadyToExecute flag returned from the contract. However, that flag was computed based on the current at the time of the call, and not based on the time in which the tx would land. This meant that we always failed to execute the slash payload on the first slot of a round.
Split l1-tx-utils into several files before going into further changes.
Can merge once approved
This PR removes `TypedOracle` and `TXETypedOracle`. In their place, it creates 5 interfaces for the different kinds of oracles: - utility - private - avm - txe - misc (a new kind, used for stuff that's available both in utility and also top level tests, such as random()) These interfaces are then implemented by the different classes that handle execution. The interfaces include runtime markers so that we can figure out whether a generic handler implements a specific interface or not, and throw during foreign call translation if the current handler doesn't support it (e.g. when calling a private oracle in the top level context in a test, or when calling a private oracle while simulating a utility function). We used to be able to always call these functions and then have their implementation error out via the default `TypedOracle` implementation, but this was a bit messy as there was no actual way to check for full interface implementation. In doing this I found some minor issues with the oracles, which already make this be valuable, e.g. there were some abandoned oracles that were just never invoked but still declared in the TXETypedOracle object. I'll do some oracle cleanup later on, but not now in order to try to make all the changes happen in one go and reduce noise there. The **real** reason why I made this change is because now `txe_oracle` can be replaced by `private_execution_oracle` and `utility_execution_oracle`, since they now implement the same interface. This will mean that Noir tests will be using the real PXE implementation and not a copy of it. I'll do this in a separate PR because there'll likely be things that need to be adjusted there. The diff is a bit messy due to interface changes combined with files being renamed. I did not actually change any of the oracle methods. Co-authored-by: Nicolás Venturo <nicolas.venturo@gmail.com>
This is now needed as we rely on bb to do avm transpilation, and disabling the AVM also disables avm-transpiler linking (which just warns, doesn't error, but maybe we keep it warning now that boxes checks this)
This pr adds a link to dwarf library in fuzzing setup whenever it is present on a system Also added the required packages for fuzzing container build Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
Initial cleanup of the circuit builders: - Removal of unused members and methods - Make member zero_idx private an provide getter `zero_idx()` - Add set_gate_selector() method to trace blocks to avoid boilerplate of selector padding in gate construction methods - Rename create_dummy_gate() to create_unconstrained_gate() (creating a gate where no selectors are active) - Make access specifiers as restrictive as possible in builder classes
Also allow lint and format on individual packages.
- One little nit about how to import Map, this is probably missing from other places so I'll add them as I go - Fixing instructions on how to see the debug_logs - Removing the .deploy method which was removed from the BaseWallet Co-authored-by: José Pedro Sousa <outgoing@zpedro.dev>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Adds a check to bootstrap such that, if the suite ends in `parallel.test.ts`, then the script extracts all test names and returns them as individual commands. Tweaks test_simple and run_test so they accept a `testNamePattern` argument.
mralj
force-pushed
the
mralj/chore/patch-chainsafe-discv5-v4
branch
from
531ba4c to
15200e1
Compare
mralj
requested review from
a team,
IlyasRidhuan,
Maddiaa0,
charlielye,
dbanks12,
fcarreiro,
jeanmon and
sirasistant
as code owners
auto-merge was automatically disabled
October 13, 2025 13:12
Pull request was closed
Contributor
Author
|
Something went really bad with my local repo, new PR (same commit applied): #17662 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Removed dependency on bigint-buffer by patching discv5.
Repo link
Applied changes are from this PR