Skip to content

Conversation

@PatStiles
Copy link
Contributor

@PatStiles PatStiles commented Nov 12, 2024

Fix: Batcher User Balance Checker

Description

Replaces the hardcoded constant in https://github.com/yetanotherco/aligned_layer/blob/testnet/batcher/aligned-batcher/src/lib.rs#L607 with the user's min_fee. Addresses the first point of #1383 .

This fix is done by adding total_fees_in_queue variable in the UserState struct, and comparing if user has enough balance to pay for the needed amount.

This PR also refactors the infamous min_fee variable, to be called max_fee_limit / last_max_fee_limit

This PR also adds some metrics of this. dont forget to visualize them in the following tests running

make run_metrics

To Test:

test 1

  • Run a local network and ensure proofs are being validated
  • Submit a single proof with no balance in the BatcherPaymentService Contract :
    1.) Start a local testnet with an no balance:
    2.) Create a wallet with no funds
cast wallet new <KEYSTORE_PATH>

3.)

echo "Sending SP1 fibonacci task to Batcher..."
cd batcher/aligned/ && cargo run --release -- submit \
		--proving_system SP1 \
		--proof ../../scripts/test_files/sp1/sp1_fibonacci.proof \
		--vm_program ../../scripts/test_files/sp1/sp1_fibonacci.elf \
		--repetitions 1 \
		--proof_generator_addr <WALLET_ADDRESS> \
		--keystore_path <KEYSTORE_PATH> \
		--rpc_url http://localhost:8545 \
		--network devnet

ensure the proof submission errors within the batcher with InsufficientBalance.

test 2

  • Submit multiple proofs and not have enough to pay.
    1.) Fund the wallet by sending eth from a prefunded anvil account:
cast send <WALLET_ADDRESS --rpc-url http://localhost:8545 --private-key 2a871d0798f97d79848a013d4936a73bf4cc922c825d33c1cf7073dff6d409c6 --value 1ether

2.) Fund the batcher with an insufficient balance for multiple proofs.

aligned deposit-to-batcher \
--rpc_url http://localhost:8545 \                      
--network devnet \ 
--keystore_path <KEYSTORE_PATH> \
--amount 0.0026ether

3.) Send one proof, see it succeeds.

cargo run --release -- submit \
                --proving_system SP1 \
                --proof ../../scripts/test_files/sp1/sp1_fibonacci.proof \
                --vm_program ../../scripts/test_files/sp1/sp1_fibonacci.elf \
                --repetitions 1 \
                --proof_generator_addr <WALLET_ADDRESS \
                --keystore_path <KEYSTORE_PATH> \
                --rpc_url http://localhost:8545 \
                --network devnet

4.) Send a burst of 10 or more proofs, observe it submission fails due to insufficient balance.

cargo run --release -- submit \
                --proving_system SP1 \
                --proof ../../scripts/test_files/sp1/sp1_fibonacci.proof \
                --vm_program ../../scripts/test_files/sp1/sp1_fibonacci.elf \
                --repetitions 10 \
                --proof_generator_addr <WALLET_ADDRESS \
                --keystore_path <KEYSTORE_PATH> \
                --rpc_url http://localhost:8545 \
                --network devnet

Uploading Screenshot 2024-11-13 at 14.03.31.png…

Type of change

Please delete options that are not relevant.

  • Refactor
  • Fix

Checklist

  • Linked to Github Issue
  • This change requires new documentation.
    • Documentation has been added/updated.
  • This change is an Optimization
    • Benchmarks added/run

@PatStiles PatStiles marked this pull request as draft November 12, 2024 20:01
@PatStiles PatStiles marked this pull request as ready for review November 13, 2024 17:24
@MauroToscano
Copy link
Contributor

Code looks good, may need some extra testing

Comment on lines 795 to 799
let mut min_fee = user_min_fee;
if user_min_fee == U256::max_value() {
min_fee = user_max_fee
}
let min_balance: U256 = U256::from(user_proofs_in_batch) * min_fee;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I am wrong here but why don't we move the min_balance check after the msg_max_fee check? This way, in the min_balance check we'll verify against the msg_max_fee directly (as it will become the new min_fee if everything goes ok), we'll save ourselves this if:

        if user_min_fee == U256::max_value() {
            min_fee = user_max_fee
        }

It is hard to understand that U256::max_value() is the default value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thoughts are to leave this check in as it handles an explicit case that a user's proof has been submitted for the first time not the replacement max_fee invariant. I we move this check behind the msg_max_fee check we still need to have logic to check and specify that if a proof is submitted for the first time we use it's submitted max_fee instead of the user_max_fee. I think it's worth considering verifying that these checks are not redundant, but lets address that in another issue.

Copy link
Member

@MarcosNicolau MarcosNicolau left a comment

Choose a reason for hiding this comment

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

I tested on macos, everything went fine.

@PatStiles PatStiles force-pushed the fix/batcher_user_balance_checks branch from cee3b0c to e242905 Compare November 14, 2024 15:02
Copy link
Contributor

@uri-99 uri-99 left a comment

Choose a reason for hiding this comment

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

the removed var, MIN_FEE_PER_PROOF, is no longer used in the codebase.
We should remove it from the constants file as well

Copy link
Contributor

@uri-99 uri-99 left a comment

Choose a reason for hiding this comment

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

the check is not accurate, it is multiplying user's amt_of_proofs x min_fee_per_proof. This is a big wrong estimation, see pic attached:
image

@uri-99
Copy link
Contributor

uri-99 commented Nov 15, 2024

Its working now but I can't approve it because i made this pr

@uri-99 uri-99 self-assigned this Nov 15, 2024
@MauroToscano MauroToscano self-requested a review November 19, 2024 17:41
@MauroToscano MauroToscano requested a review from uri-99 November 19, 2024 18:17
Copy link
Member

@MarcosNicolau MarcosNicolau left a comment

Choose a reason for hiding this comment

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

Tested on macos, everything worked fine. Code looks good, I leave some comments that you can address if you want.

@MarcosNicolau MarcosNicolau self-requested a review November 19, 2024 18:52
@uri-99
Copy link
Contributor

uri-99 commented Nov 19, 2024

1 sec testing something

@uri-99 uri-99 added this pull request to the merge queue Nov 20, 2024
Merged via the queue into staging with commit c223d8f Nov 20, 2024
3 checks passed
@uri-99 uri-99 deleted the fix/batcher_user_balance_checks branch November 20, 2024 14:22
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