feat: prune used merkle proof #218

Lazar955 · 2024-12-12T15:13:22Z

Lazar955 · 2024-12-12T15:34:20Z

Hm, it's expected for double sign test to fail now, as the proof is deleted

gitferry

Keep thinking about this. I feel safer if we make it a CLI: fpd unsafe-prune-merkle-proof [eots-pk] --chain-id --up-to-height where --chain-id is required. If the user does not specify --up-to-height, the pruning is up to the fp's last voted height.

This would require db schema change as follows:

the current key is the randomness number itself, which does not support range query by height. Therefore we need to add height to the key
given that the fpd might store different fps with different chain id, we need also add chainid and fp public key in the key like we did for signing record

finality-provider/eotsmanager/store/storedsigningrecord.go

Line 22 in 27349c9

func getSignRecordKey(chainID, pk []byte, height uint64) []byte {

wdyt?

bap2pecs · 2024-12-13T02:23:40Z

Keep thinking about this. I feel safer if we make it a CLI: fpd unsafe-prune-merkle-proof [eots-pk] --chain-id --up-to-height where --chain-id is required. If the user does not specify --up-to-height, the pruning is up to the fp's last voted height.

This would require db schema change as follows:

the current key is the randomness number itself, which does not support range query by height. Therefore we need to add height to the key

given that the fpd might store different fps with different chain id, we need also add chainid and fp public key in the key like we did for signing record

finality-provider/eotsmanager/store/storedsigningrecord.go

Line 22 in 27349c9

func getSignRecordKey(chainID, pk []byte, height uint64) []byte {

wdyt?

i am not familiar with this db schema but the first point makes sense.

for 2), I don't know if it's necessary to change db because a FP can only register for one consumer chain. So we can simply query the FP's consumer chain id to see if it matches with what's passed in to the CLI

meta-thought: you guys might need to think about data migration for backward compatibility issue.

gitferry · 2024-12-13T02:45:05Z

for 2), I don't know if it's necessary to change db because a FP can only register for one consumer chain. So we can simply query the FP's consumer chain id to see if it matches with what's passed in to the CLI

We need to change db to add height as the key for each randomness along with the proof. However, a fpd may store randomness for different fps, so fp public key is also neede in the key. Probably we don't need chainid as the key for now as fp is unique across different chains but I think it's worth keeping it for potential range query for different chains

meta-thought: you guys might need to think about data migration for backward compatibility issue.

my understanding for this is that after we launch phase-2, for any db schema change later on we need to consider backward compatibility. Do you think we need to do anything before the launch?

bap2pecs · 2024-12-13T04:12:19Z

my understanding for this is that after we launch phase-2, for any db schema change later on we need to consider backward compatibility. Do you think we need to do anything before the launch?

yeah i think it makes sense to only consider it after mainnet launch.

RafilxTenfen · 2024-12-13T14:30:34Z

finality-provider/cmd/fpd/daemon/daemon_commands.go

@@ -488,6 +488,63 @@ func runCommandEditFinalityDescription(cmd *cobra.Command, args []string) error
 	return nil
 }

+// CommandUnsafeDeleteMerkleProof removes merkle proof
+func CommandUnsafeDeleteMerkleProof() *cobra.Command {
+	var cmd = &cobra.Command{


should we have a command to delete this? kkk seems a bit scary

Yeah, maybe more docs about the consequences?

I would use prune other than remove which seems less scary. Let's add a long description of the consequences. The user should ensure that up to the pruning height, the fp has already voted or it does not have voting power for those blocks

RafilxTenfen · 2024-12-13T14:48:23Z

finality-provider/store/pub_rand.go

@@ -133,4 +171,37 @@ func (s *PubRandProofStore) GetPubRandProofList(pubRandList []*btcec.FieldVal) (
 	return proofBytesList, nil
 }

-// TODO: delete function?
+// RemovePubRandProofList removes all proofs up to the target height
+func (s *PubRandProofStore) RemovePubRandProofList(chainID []byte, pk []byte, targetHeight uint64) error {


lets add a specific unit test for this one?

thanks, added in 514b202

RafilxTenfen

utACK, suggestion to add a specific unit test for RemovePubRandProofList

gitferry

Great work!

gitferry · 2024-12-14T15:14:51Z

finality-provider/cmd/fpd/daemon/daemon_commands.go

@@ -488,6 +488,63 @@ func runCommandEditFinalityDescription(cmd *cobra.Command, args []string) error
 	return nil
 }

+// CommandUnsafeDeleteMerkleProof removes merkle proof
+func CommandUnsafeDeleteMerkleProof() *cobra.Command {
+	var cmd = &cobra.Command{


I would use prune other than remove which seems less scary. Let's add a long description of the consequences. The user should ensure that up to the pruning height, the fp has already voted or it does not have voting power for those blocks

gitferry · 2024-12-16T07:49:45Z

finality-provider/service/fp_instance.go

@@ -1020,3 +1026,8 @@ func (fp *FinalityProviderInstance) GetFinalityProviderSlashedOrJailedWithRetry(

 	return slashed, jailed, nil
 }
+
+// TestGetPubProof only used for tests to get access to the store
+func (fp *FinalityProviderInstance) TestGetPubProof(height uint64) ([]byte, error) {


This is probably not needed as we can access the store by app.GetPubRandProofStore()

* Init * Update babylon-private reference * Revert "CI: Remove redundant SSH key logic (#218)"

prune proof

6181eae

Lazar955 requested a review from gitferry December 12, 2024 15:13

clog

6cb8ce1

Lazar955 linked an issue Dec 12, 2024 that may be closed by this pull request

Prune used merkle proof from db #194

Closed

gitferry reviewed Dec 13, 2024

View reviewed changes

Lazar955 added 6 commits December 13, 2024 10:39

change pubrand key

d667c30

remove up to the target height

ca06140

rpc methods

999104b

adds cmd for removing proofs

36ceef5

adds e2e for deleting merkle proof

542ba04

proper e2e

406c507

Lazar955 requested review from gitferry and RafilxTenfen December 13, 2024 13:45

Lazar955 added 2 commits December 13, 2024 14:53

fix order

5d323c0

gosec

db58714

Lazar955 changed the title ~~chore: prune used merkle proof~~ feat: prune used merkle proof Dec 13, 2024

RafilxTenfen reviewed Dec 13, 2024

View reviewed changes

RafilxTenfen approved these changes Dec 13, 2024

View reviewed changes

adds unit test for merkle proof removal

514b202

gitferry approved these changes Dec 16, 2024

View reviewed changes

pr comments

66af147

Lazar955 merged commit d8506b6 into main Dec 16, 2024
12 checks passed

Lazar955 deleted the lazar/prune-proof branch December 16, 2024 10:31

filippos47 pushed a commit that referenced this pull request Feb 10, 2025

Consumer chain finality provider (#276)

b79ac31

* Init * Update babylon-private reference * Revert "CI: Remove redundant SSH key logic (#218)"

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: prune used merkle proof #218

feat: prune used merkle proof #218

Lazar955 commented Dec 12, 2024

Lazar955 commented Dec 12, 2024

gitferry left a comment

bap2pecs commented Dec 13, 2024

gitferry commented Dec 13, 2024

bap2pecs commented Dec 13, 2024

RafilxTenfen Dec 13, 2024

Lazar955 Dec 13, 2024

gitferry Dec 14, 2024

RafilxTenfen Dec 13, 2024

Lazar955 Dec 13, 2024

RafilxTenfen left a comment

gitferry left a comment

gitferry Dec 14, 2024

gitferry Dec 16, 2024

feat: prune used merkle proof #218

feat: prune used merkle proof #218

Conversation

Lazar955 commented Dec 12, 2024

Lazar955 commented Dec 12, 2024

gitferry left a comment

Choose a reason for hiding this comment

bap2pecs commented Dec 13, 2024

gitferry commented Dec 13, 2024

bap2pecs commented Dec 13, 2024

RafilxTenfen Dec 13, 2024

Choose a reason for hiding this comment

Lazar955 Dec 13, 2024

Choose a reason for hiding this comment

gitferry Dec 14, 2024

Choose a reason for hiding this comment

RafilxTenfen Dec 13, 2024

Choose a reason for hiding this comment

Lazar955 Dec 13, 2024

Choose a reason for hiding this comment

RafilxTenfen left a comment

Choose a reason for hiding this comment

gitferry left a comment

Choose a reason for hiding this comment

gitferry Dec 14, 2024

Choose a reason for hiding this comment

gitferry Dec 16, 2024

Choose a reason for hiding this comment