add instruction to close violation report #19

AshwinSekar · 2025-02-11T02:08:52Z

After a violation report has been written and at least 3 epochs have passed, anyone can close the report account and credit the lamports to the destination account specified on the report.

Requires write access to the report account, and the destination account and then:

Check that we own the report account and it is not empty
Check that current epoch >= report epoch + 3
Check that the destination account matches the destination specified on the report
Realloc the report account to 0 bytes and assign the system program as the owner
Transfer all the lamports from report account to the destination account

program/src/processor.rs

AshwinSekar · 2025-02-11T02:15:18Z

program/src/lib.rs

@@ -37,3 +37,51 @@ pub fn get_violation_report_address(
        &id(),
    )
 }
+
+struct ViolationReportAddress<'a> {


not sure if me trying to reuse some code made this better or worse, happy to delete this if you think worse.

Looks good actually! My only comment would be to move it to another file like state.rs or create address.rs since it's only used internally.

Moved it to a separate file here f9ab512
After removing the system program from CloseViolationReport, we actually don't need it there, so there's only one consumer. I've kept it here in case we need it in the future.

program/tests/duplicate_block_proof.rs

joncinque

Looks good overall! Mostly nits and a way to simplify the logic a bit (I hope)

joncinque · 2025-02-14T21:37:52Z

program/src/instruction.rs

+    ///    `[get_violation_report_address]` for the address derivation.
+    /// 1. `[WRITE]` Destination account which will be credited the lamports
+    ///    from the PDA.
+    /// 2. `[]` System program


This shouldn't be needed

removed it here 754ad31

joncinque · 2025-02-14T21:38:46Z

program/src/instruction.rs

+    encode_instruction(
+        accounts,
+        SlashingInstruction::CloseViolationReport,
+        &[0u8; 0],


nit: is it possible to use some form of () instead?

good point, used &() instead 68e2d6a

program/src/processor.rs

joncinque · 2025-02-14T21:45:47Z

program/src/lib.rs

@@ -37,3 +37,51 @@ pub fn get_violation_report_address(
        &id(),
    )
 }
+
+struct ViolationReportAddress<'a> {


Looks good actually! My only comment would be to move it to another file like state.rs or create address.rs since it's only used internally.

joncinque · 2025-02-14T21:48:51Z

program/src/state.rs

+    let transfer_instruction = system_instruction::transfer(&report_key, &destination, lamports);
+    invoke_signed(
+        &transfer_instruction,
+        &[
+            report_account.clone(),
+            destination_account.clone(),
+            system_program_account.clone(),
+        ],
+        &[&seeds],
+    )?;


It'll be much easier to just subtract the lamports directly from report_account and then realloc / reassign, and that way there's no need to CPI to the system program.

Check out how the token program does this: https://github.com/solana-program/token/blob/08aa3ccecb30692bca18d6f927804337de82d5ff/program/src/processor.rs#L702-L708

We'll just need some special logic to do nothing if the destination is the report account itself, so that accounts are always closable. Alternatively, we can make sure the destination isn't the report account during creation

That's much easier! Removed it here 754ad31

And good point, i decided to return an error if your destination is equal to the report address on report creation and added a test 3dfef7d

joncinque · 2025-02-14T22:11:47Z

program/tests/duplicate_block_proof.rs

+    context
+        .banks_client
+        .simulate_transaction(transaction.clone())
+        .await?
+        .result
+        .unwrap()?;
+    context
+        .banks_client
+        .process_transaction(transaction)
+        .await?;


What's the point of simulating before processing? You should get an error back either way if the transaction fails

I was running into what I believe was status cache issues because I was sending the same close report transaction with the same block hash while updating the clock sysvar. Got lazy and put in this hack to simulate instead 😅 .

Turns out there's a warp_to_epoch that does the proper setup of the new bank so we don't need this hack and also removes the need for incrementing the clock's epoch field manually cc8d301

joncinque · 2025-02-14T22:12:36Z

program/tests/duplicate_block_proof.rs

+    context: &mut ProgramTestContext,
+    report_key: Pubkey,
+    destination: Pubkey,
+    initial_lamports: u64,


Up to you, but rather than passing this in, it could be fetched from the report account in this function

Saves us an arg, fetched it instead 68e2d6a

joncinque · 2025-02-14T22:13:29Z

program/tests/duplicate_block_proof.rs

    let err = context
        .banks_client
-        .process_transaction(transaction)
+        .simulate_transaction(transaction)
        .await
-        .unwrap_err()
-        .unwrap();
+        .unwrap()
+        .result
+        .unwrap()
+        .unwrap_err();


Was this change intended?

Not intended, this is leftover from when I was testing out simulate transaction for the hack, removed it and the ones below

joncinque · 2025-02-14T22:14:00Z

program/tests/duplicate_block_proof.rs

    let err = context
        .banks_client
-        .process_transaction(transaction)
+        .simulate_transaction(transaction)
        .await
-        .unwrap_err()
-        .unwrap();
+        .unwrap()
+        .result
+        .unwrap()
+        .unwrap_err();


Same here, was this change intended?

joncinque · 2025-02-14T22:17:33Z

program/tests/duplicate_block_proof.rs

+    // Close report should fail with invalid destination account
+    let err = close_report(
+        &mut context,
+        report_key,
+        Pubkey::new_unique(),
+        report_account.lamports,
+        3,
+    )
+    .await
+    .unwrap_err()
+    .unwrap();


Not a big deal, but this is missing a call to setup_clock before calling close_report

joncinque

Looks great, thanks for integrating all the feedback!

add instruction to close violation report

7319d29

AshwinSekar commented Feb 11, 2025

View reviewed changes

AshwinSekar requested a review from joncinque February 11, 2025 02:21

joncinque reviewed Feb 14, 2025

View reviewed changes

AshwinSekar added 5 commits February 15, 2025 03:17

pr feedback: directly increment lamports rather than CPI

754ad31

pr feedback: use &(), increment_clock_epoch, pass in lamps, setup_clock

68e2d6a

pr feedback: error if destination is equal to report address

3dfef7d

pr feedback: remove simulation hack, use warp_to_epoch

cc8d301

pr feedback: move address struct to separate file

f9ab512

AshwinSekar requested a review from joncinque February 15, 2025 06:16

joncinque approved these changes Feb 25, 2025

View reviewed changes

AshwinSekar merged commit 9330097 into solana-program:main Feb 25, 2025
7 checks passed

AshwinSekar deleted the close-report branch February 25, 2025 19:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add instruction to close violation report #19

add instruction to close violation report #19

AshwinSekar commented Feb 11, 2025

AshwinSekar Feb 11, 2025

joncinque Feb 14, 2025

AshwinSekar Feb 15, 2025

joncinque left a comment

joncinque Feb 14, 2025

AshwinSekar Feb 15, 2025

joncinque Feb 14, 2025

AshwinSekar Feb 15, 2025

joncinque Feb 14, 2025

joncinque Feb 14, 2025

AshwinSekar Feb 15, 2025 •

edited

Loading

joncinque Feb 14, 2025

AshwinSekar Feb 15, 2025 •

edited

Loading

joncinque Feb 14, 2025

AshwinSekar Feb 15, 2025

joncinque Feb 14, 2025

AshwinSekar Feb 15, 2025

joncinque Feb 14, 2025

joncinque Feb 14, 2025

joncinque left a comment

add instruction to close violation report #19

add instruction to close violation report #19

Conversation

AshwinSekar commented Feb 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joncinque 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

AshwinSekar Feb 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AshwinSekar Feb 15, 2025 • edited Loading

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

joncinque left a comment

Choose a reason for hiding this comment

AshwinSekar Feb 15, 2025 •

edited

Loading

AshwinSekar Feb 15, 2025 •

edited

Loading