feat(genLocalState) - debugging utility for log fetching and local maci state generation #758

ctrlc03 · 2023-10-20T10:33:17Z

fix #723
fix #447

Fix bug of double fetching
Add fetching in blocks for generateMaciState
Implement serialization and deserialization for all DomainObjects as well as MaciState and Poll
In genLocalState save the serialized MaciState and Polls to a JSON file
in genProofs add the option to read the state from a json file -> deserialize the json file into a valid MaciState and Polls

How to test:

Run tests/vanilla/testPreFetch.sh

cli/ts/genProofs.ts

baumstern · 2023-11-01T03:39:11Z

Isn't #723 a request to add the fetchLogs command back?

baumstern · 2023-11-01T03:47:08Z

contracts/ts/genMaciState.ts

+    const lastBlock = endBlock ? endBlock : await provider.getBlockNumber()
+
+    // Fetch event logs in batches
+    for (let i = fromBlock; i < lastBlock; i += blocksPerRequest + 1) {


I believe it's worthwhile to implement test code to avoid issues like the previous case of duplicate logs. Please note that such duplicate logs resulted in generating an incorrect proof.

// init() should only be called up to 1 time assert( initLogs.length <= 1, 'More than 1 init() event detected which should not be possible', )

If there are duplicate logs, this will throw.

There are signUpLogs, mergeStateAqSubRootsLogs, mergeStateAqLogs, deployPollLogs, publishMessageLogs...etc. Without adequate tests, confidently predicting the impact of these changes on production becomes challenging. What are your thoughts on this?

this code incorporates the fix needed to ensure we do not fetch duplicate logs - the same fix that the project encountering the issue proposed + We do have e2e and integration tests which use this genProof and have been working correctly.

ctrlc03 · 2023-11-01T09:54:31Z

Isn't #723 a request to add the

sure, but there was missing context about that, fetchlogs was replaced by this genMaciStateFromContract in v1. Only thing missing was the ability to fetch in batches

baumstern · 2023-11-02T03:06:13Z

Isn't #723 a request to add the

sure, but there was missing context about that, fetchlogs was replaced by this genMaciStateFromContract in v1. Only thing missing was the ability to fetch in batches

It’s challenging to view genMaciStateFromContract() as a direct replacement for fetchLogs() considering the following operational aspects:

File Output Capability: The original fetchLogs() provides a feature to output logs to a file. This is particularly important when dealing with extensive datasets, ensuring that we can pre-fetch logs and pass them onto the genProofs().

Code Reference: fetchLogs usage

Independent Usability for Debugging: fetchLogs() offers the advantage of being independently usable within the CLI, alongside the genProofs() command. This independence is not just about providing flexibility; it plays a critical role in debugging and verification. Given the historical context, such as the ethStaker round last year where we witnessed genProof() command failures and experienced runtimes extending beyond 10 hours, having the ability to independently run and validate the output of fetchLogs becomes invaluable.

baumstern

The fetchLogs command, as it's currently implemented, seems to be misnamed as it does more than just fetching logs—it generates and outputs the MACI state, which is an entirely different responsibility. This oversteps the expected functionality of a log-fetching utility, leading to unnecessary complexity and potential confusion:

The command requires contract ABIs and the coordinator's private key, which are beyond the scope of simply fetching logs. These requirements suggest operations relevant to MACI state generation and proof creation, not log retrieval.
It performs operations with significant time complexity, such as array pushes and sorts on the entire log dataset, which are superfluous for log fetching; these operations incurs O(n) and O(n log n) time complexities.
The inclusion of the coordinator's private key raises security concerns, as it's not necessary for fetching logs and should not be requested without cause

This conflation of concerns could be misleading and might introduce inefficiencies or security vulnerabilities. A command should do one thing and do it well—fetching logs shouldn't involve generating state. Consider refactoring to separate these concerns, ensuring the command name and its functionality are in alignment, and that it adheres to the principle of least privilege by not requiring sensitive information where it's not needed.

cli/ts/fetchLogs.ts

baumstern · 2023-11-08T05:49:55Z

contracts/ts/genMaciState.ts

+    for (let i = fromBlock; i < lastBlock; i += blocksPerRequest + 1) {
+        const toBlock = (i + blocksPerRequest) >= lastBlock ? undefined : i + blocksPerRequest
+
+        const tmpInitLogs = await provider.getLogs({


Consider (Rate Limiting): It's worth noting that this PR addresses the eth_getLogs rate limit by splitting up requests to stay within the 10K logs response cap. However, Alchemy also implements rate limiting through Compute Units Per Second (CUPS), which applies to the API key as a whole. While this PR doesn't need to address CUPS limits right now, it's an important aspect to keep in mind for the future to ensure our application remains within Alchemy's operational boundaries (or generally other node provider services as well).

Added a sleep flag (optional) which users can use to add a sleep in between each getLog request :) - great suggestion thanksss

baumstern · 2023-11-08T06:21:20Z

cli/ts/fetchLogs.ts

+
+    // ensure we have at least one address
+    if ((!contractAddrs||!contractAddrs["MACI"]||!contractAddrs[pollArtifactName]) && !args.maci_contract && !args.poll_contract) {
+        console.error('Error: MACI and Poll contract addresses are empty or this poll Id does not exist') 


This error message does not guide the user on how to resolve the issue. It would be beneficial to specify which command-line arguments the user needs to pass to rectify this error. For example, we could enhance the error message to something like:

Error: MACI and Poll contract addresses are empty or this poll Id does not exist. Please ensure you include '--maci-contract' and '--poll-contract' with the respective contract addresses as arguments.

This clarification would help users to self-diagnose and fix the issue without delving into the codebase or documentation.

baumstern · 2023-11-08T06:24:41Z

cli/ts/fetchLogs.ts

+    const pollArtifactName = `Poll-${pollId}`
+
+    // ensure we have at least one address
+    if ((!contractAddrs||!contractAddrs["MACI"]||!contractAddrs[pollArtifactName]) && !args.maci_contract && !args.poll_contract) {


This conditional check seems to allow the execution to proceed even when poll_contract is not provided, which results in a TypeError at the address validation step:

% node build/index.js fetchLogs -n 150 -o fetchlogs.log -sk 'macisk.aa' -x '0x37091e3064c20637950e007500049477B0b96636' /Users/daehyun/workspace/pse/maci/cli/ts/utils.ts:33 return address.match(/^0x[a-fA-F0-9]{40}$/) != null ^ TypeError: Cannot read properties of undefined (reading 'match') at validateEthAddress (/Users/daehyun/workspace/pse/maci/cli/ts/utils.ts:33:20) at /Users/daehyun/workspace/pse/maci/cli/ts/fetchLogs.ts:143:28 at step (/Users/daehyun/workspace/pse/maci/cli/build/fetchLogs.js:33:23) at Object.next (/Users/daehyun/workspace/pse/maci/cli/build/fetchLogs.js:14:53) at /Users/daehyun/workspace/pse/maci/cli/build/fetchLogs.js:8:71 at new Promise (<anonymous>) at __awaiter (/Users/daehyun/workspace/pse/maci/cli/build/fetchLogs.js:4:12) at fetchLogs (/Users/daehyun/workspace/pse/maci/cli/ts/fetchLogs.ts:121:35) at /Users/daehyun/workspace/pse/maci/cli/ts/index.ts:249:24 at step (/Users/daehyun/workspace/pse/maci/cli/build/index.js:34:23) at Object.next (/Users/daehyun/workspace/pse/maci/cli/build/index.js:15:53) at /Users/daehyun/workspace/pse/maci/cli/build/index.js:9:71 at new Promise (<anonymous>) at __awaiter (/Users/daehyun/workspace/pse/maci/cli/build/index.js:5:12) at main (/Users/daehyun/workspace/pse/maci/cli/ts/index.ts:137:14) at Object.<anonymous> (/Users/daehyun/workspace/pse/maci/cli/ts/index.ts:254:5)

To prevent this runtime error, we should consider making the poll_contract argument mandatory or add a more descriptive error message when it's missing. This would improve the user experience by providing immediate and clear feedback on what is required for successful execution.

mm if you do not pass a poll ID, it will default to poll ID number 0.. where are you trying this? on a local network or on a live testnet?

baumstern · 2023-11-08T06:26:42Z

cli/ts/fetchLogs.ts

+    )
+
+    parser.addArgument(
+        ['-e', '--eth-provider'],


The --eth-provider argument is essential for the operation of this script as it stands. Omitting this results in a NETWORK_ERROR, indicating that the script cannot detect the network.

% node build/index.js fetchLogs -n 150 -o fetchlogs.log -sk 'macisk.aaa' -x '0x37091e3064c20637950e007500049477B0b96636' -p '0x37091e3064c20637950e007500049477B0b96636' /Users/daehyun/workspace/pse/maci/cli/node_modules/@ethersproject/logger/src.ts/index.ts:269 const error: any = new Error(message); ^ Error: could not detect network (event="noNetwork", code=NETWORK_ERROR, version=providers/5.7.2) at Logger.makeError (/Users/daehyun/workspace/pse/maci/cli/node_modules/@ethersproject/logger/src.ts/index.ts:269:28) at Logger.throwError (/Users/daehyun/workspace/pse/maci/cli/node_modules/@ethersproject/logger/src.ts/index.ts:281:20) at JsonRpcProvider.<anonymous> (/Users/daehyun/workspace/pse/maci/cli/node_modules/@ethersproject/providers/src.ts/json-rpc-provider.ts:483:23) at step (/Users/daehyun/workspace/pse/maci/cli/node_modules/@ethersproject/providers/lib/json-rpc-provider.js:48:23) at Object.throw (/Users/daehyun/workspace/pse/maci/cli/node_modules/@ethersproject/providers/lib/json-rpc-provider.js:29:53) at rejected (/Users/daehyun/workspace/pse/maci/cli/node_modules/@ethersproject/providers/lib/json-rpc-provider.js:21:65) at processTicksAndRejections (node:internal/process/task_queues:95:5)

Could we adjust the argument's configuration to be mandatory?

could it be that you were not running a hardhat local node? if you do not pass it as argument then it will default to the local rpc.. if no local node is running it will ofc give this error

baumstern · 2023-11-08T09:33:14Z

cli/ts/fetchLogs.ts

+    maciPrivkeyGroup.addArgument(
+        ['-sk', '--privkey'],


Considering the main functionality of fetchLogs command is to retrieve event logs, including a private key argument might not be required. Removing it could simplify the command-line interface and minimize potential security concerns.

this is needed to generate the maci state :)

cli/ts/fetchLogs.ts

baumstern · 2023-11-08T09:53:07Z

cli/ts/fetchLogs.ts

+    // fetch abis
+    const [ maciContractAbi ] = parseArtifact('MACI')
+    const [ pollContractAbi ] = parseArtifact('Poll')
+
+    const maciContract = new ethers.Contract(
+        maciAddress,
+        maciContractAbi,
+        provider,
+    )
+
+    const pollContract = new ethers.Contract(
+        pollAddress,
+        pollContractAbi,
+        provider
+    )


maciContract and pollContract instances are created here but do not appear to be used later in the code. If these instances are not required for subsequent operations, their creation could unnecessarily consume resources and possibly lead to confusion about their purpose. It might be worth considering their removal to streamline the codebase unless there is a future use for them that is not immediately apparent. This would also help in maintaining clean and efficient code.

removed thanks

baumstern · 2023-11-08T10:05:36Z

cli/ts/genProofs.ts

+    if (args.state_file) {
+        // @todo actually read the file first 
+        const content = JSON.parse(fs.readFileSync(args.state_file, 'utf-8').toString())
+        maciState = MaciState.fromJSON(content)
+        // ensure we merge all messages
+        maciState.polls.forEach((poll: Poll) => poll.mergeAllMessages()) 
+    } else {


This change introduce to reading from a state file if args.state_file is provided, and then generating the proof based on the JSON content. However, there appears to be no test coverage to verify the correctness of the serialization/deserialization process and the subsequent proof generation using the parsed data. Could we introduce unit test that validate the correct functioning of the proof generation when using parsed data?

Sure I can add one e2e! unit tests for deserialization and serialization are in the core package btw

baumstern · 2023-11-08T10:07:03Z

cli/ts/genProofs.ts

-        fromBlock = txn.blockNumber
+    if (args.state_file) {
+        // @todo actually read the file first 
+        const content = JSON.parse(fs.readFileSync(args.state_file, 'utf-8').toString())


This assumes that the input file format is correct. It's important to handle potential exceptions that could arise from incorrect file formats. Adding error handling for the JSON parsing process could prevent runtime errors and improve the robustness of the script.

ctrlc03 · 2023-11-09T15:32:20Z

The fetchLogs command, as it's currently implemented, seems to be misnamed as it does more than just fetching logs—it generates and outputs the MACI state, which is an entirely different responsibility. This oversteps the expected functionality of a log-fetching utility, leading to unnecessary complexity and potential confusion:

The command requires contract ABIs and the coordinator's private key, which are beyond the scope of simply fetching logs. These requirements suggest operations relevant to MACI state generation and proof creation, not log retrieval.

It performs operations with significant time complexity, such as array pushes and sorts on the entire log dataset, which are superfluous for log fetching; these operations incurs O(n) and O(n log n) time complexities.

The inclusion of the coordinator's private key raises security concerns, as it's not necessary for fetching logs and should not be requested without cause

This conflation of concerns could be misleading and might introduce inefficiencies or security vulnerabilities. A command should do one thing and do it well—fetching logs shouldn't involve generating state. Consider refactoring to separate these concerns, ensuring the command name and its functionality are in alignment, and that it adheres to the principle of least privilege by not requiring sensitive information where it's not needed.

Ok so the file now has a different name, generateLocalState to avoid confusion. It is not a 100% replacement for fetchLogs which was present in v0.x but it is a new utility for Maci v.1x - it provides support for fetching logs in chunks, sleep option for avoiding rate limiting of RPC providers, and the ability to save the full state in a nice and optimal JSON representation. This allows for better debugging, as it can be analyzed separately if any issues arise when generating proofs.

baumstern · 2023-11-13T16:16:49Z

cli/tests/prepare_test.sh

+
+    echo "gen proofs ..."
+
+    # generate the local state


Nice demonstration of how to use new command!

…contract address

…r batch and end block

…we can serialize

…rialization into a new object

…sure we read the file in genProofs

…s correctly and fix bug in curentMessageBatchIndex

… limiting, change utility name, add better error handling

…e tests

…the MaciState constructor

…t as parameter when de-serializing the object

…keep one for each loop iteration

…setter function for it

… packagess

baumstern

LGTM!

ctrlc03 changed the title ~~feat(fetch-logs) - add back fetch log and adapt to MACI v1~~ feat(fetch-logs) - fetch logs in batches v1 Oct 20, 2023

ctrlc03 linked an issue Oct 23, 2023 that may be closed by this pull request

Missing MACI CLI, fetchLog, in v1.1.1 #723

Closed

ctrlc03 mentioned this pull request Oct 23, 2023

Couldn't get a large amount of logs from Alchemy #447

Closed

ctrlc03 requested review from chaosma and removed request for chaosma October 23, 2023 17:27

ctrlc03 self-assigned this Oct 23, 2023

ctrlc03 mentioned this pull request Oct 24, 2023

Missing MACI CLI, fetchLog, in v1.1.1 #723

Closed

ctrlc03 marked this pull request as ready for review October 26, 2023 12:15

ctrlc03 requested review from baumstern, samajammin and 0xjei October 26, 2023 12:15

baumstern reviewed Nov 1, 2023

View reviewed changes

cli/ts/genProofs.ts Show resolved Hide resolved

baumstern reviewed Nov 1, 2023

View reviewed changes

ctrlc03 force-pushed the feat/fetchlogs branch from 29e3946 to b566bc0 Compare November 1, 2023 09:52

ctrlc03 requested a review from baumstern November 1, 2023 18:40

ctrlc03 force-pushed the feat/fetchlogs branch from b566bc0 to 9752368 Compare November 2, 2023 15:55

ctrlc03 marked this pull request as draft November 2, 2023 18:18

ctrlc03 marked this pull request as ready for review November 7, 2023 16:34

ctrlc03 force-pushed the feat/fetchlogs branch from 1a2a444 to 4b6117e Compare November 7, 2023 16:35

ctrlc03 mentioned this pull request Nov 7, 2023

Core package refactoring #792

Closed

8 tasks

baumstern suggested changes Nov 8, 2023

View reviewed changes

ctrlc03 changed the title ~~feat(fetch-logs) - fetch logs in batches v1~~ feat(generateLocalState) - debugging utility for log fetching and local maci state generation Nov 9, 2023

ctrlc03 force-pushed the feat/fetchlogs branch from 4b6117e to 808ac9d Compare November 9, 2023 15:38

ctrlc03 requested a review from baumstern November 9, 2023 15:38

ctrlc03 changed the title ~~feat(generateLocalState) - debugging utility for log fetching and local maci state generation~~ feat(genLocalState) - debugging utility for log fetching and local maci state generation Nov 9, 2023

baumstern reviewed Nov 13, 2023

View reviewed changes

ctrlc03 added 25 commits November 28, 2023 09:28

feat(fetch-logs) - add back fetch log and adapt to MACI v1

03e4aa7

feat(fetch logs) - update genMaciState to fetch logs in batches

bd0f964

refactor(fetchlogs) - remove fetchlogs in favor of genMaciState

3d3d52e

fix(logs) - do not make start block mandatory

327e2ea

fix(fetchlogs) - ensure we set a end block for fetching logs

962eea4

fix(fetchlogs) - ensure we are fetching logs coming from our desired …

7267aa2

…contract address

fix(contracts) - fix bug in genMaciState causing duplicate log fetching

cc69e46

feat(fetchlogs) - ensure to use cli parameters to determine blocks pe…

e9acf83

…r batch and end block

feat(fetchlogs) - add maci state serialization

887dc59

feat(fetchlogs) - add toJSON method to various domainobjs classes so …

ae72108

…we can serialize

feat(fetchlogs) - add more serialization and deserialization methods

8788fd7

feat(fetchlogs) - ensure Poll equals function returns true after dese…

f19c5fd

…rialization into a new object

feat(fetchlogs) - fix wrong deserialization of Keypair objects and en…

530d7cb

…sure we read the file in genProofs

feat(fetchlogs) - ensure that we are deserializing the Command object…

a1a62ac

…s correctly and fix bug in curentMessageBatchIndex

feat(genLocalState) - add e2e test, add sleep feature to prevent rate…

80255e6

… limiting, change utility name, add better error handling

feat(fetchlogs) - update documentation to show usage of the new commands

e9a4d36

feat(genLocalState) - ensure we are deleting the test file in the cor…

f7eb5a3

…e tests

feat(genLocalState) - fix issues with package-lock files

f67b56f

feat(genLocalState) - ensure we are passing the correct arguments to …

ff73fd1

…the MaciState constructor

feat(genLocalState) - do not store coordinator key but instead take i…

b0d0d0d

…t as parameter when de-serializing the object

feat(genLocalState) - remove sleeps in between each log request, but …

efe151f

…keep one for each loop iteration

feat(genLocalState) - remove coordinator key from fromJSON and add a …

1cd4e27

…setter function for it

feat(genLocalState) - sort out conflicts and ensure code runs correctly

c56a558

feat(genLocalState) - fix corrupted package-lock file

61b2591

feat(genLocalState) - regenerate package-lock files

5002078

ctrlc03 force-pushed the feat/fetchlogs branch from 47eb6f8 to 5002078 Compare November 28, 2023 09:31

feat(genLocalState) - ensure @types/chai version is consistent across…

f8e3c4a

… packagess

baumstern approved these changes Nov 28, 2023

View reviewed changes

baumstern merged commit ca82385 into dev Nov 28, 2023
8 checks passed

baumstern deleted the feat/fetchlogs branch November 28, 2023 15:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(genLocalState) - debugging utility for log fetching and local maci state generation #758

feat(genLocalState) - debugging utility for log fetching and local maci state generation #758

ctrlc03 commented Oct 20, 2023 •

edited

Loading

baumstern commented Nov 1, 2023

baumstern Nov 1, 2023

ctrlc03 Nov 1, 2023

baumstern Nov 2, 2023

ctrlc03 Nov 2, 2023

ctrlc03 commented Nov 1, 2023

baumstern commented Nov 2, 2023 •

edited

Loading

baumstern left a comment

baumstern Nov 8, 2023

ctrlc03 Nov 9, 2023

baumstern Nov 8, 2023

baumstern Nov 8, 2023

ctrlc03 Nov 9, 2023

baumstern Nov 8, 2023

ctrlc03 Nov 9, 2023

baumstern Nov 8, 2023

ctrlc03 Nov 9, 2023

baumstern Nov 8, 2023

ctrlc03 Nov 9, 2023

baumstern Nov 8, 2023

ctrlc03 Nov 9, 2023

baumstern Nov 8, 2023

ctrlc03 commented Nov 9, 2023

baumstern Nov 13, 2023

baumstern left a comment

feat(genLocalState) - debugging utility for log fetching and local maci state generation #758

feat(genLocalState) - debugging utility for log fetching and local maci state generation #758

Conversation

ctrlc03 commented Oct 20, 2023 • edited Loading

baumstern commented Nov 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctrlc03 commented Nov 1, 2023

baumstern commented Nov 2, 2023 • edited Loading

baumstern left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctrlc03 commented Nov 9, 2023

Choose a reason for hiding this comment

baumstern left a comment

Choose a reason for hiding this comment

ctrlc03 commented Oct 20, 2023 •

edited

Loading

baumstern commented Nov 2, 2023 •

edited

Loading