Combine owners and nonce tables

[petarjuki7]

Issue Addressed

Addresses Issue #385

Proposed Changes

There is now one table owners which has both the fee_recipient and nonce fields.

Additional Info

Removed NOT NULL on fee_recipient, had to do it because on the BUMP_NONCE command we are only inserting on owner and nonce.
Tested with cargo test in the database crate and all the tests pass.

Side comment:
As far as I can tell cargo test doesn't work on the stable branch, because in the top level Cargo.toml there is a sync feature missing in the tokio dependency import.

[cla-assistant]

All committers have signed the CLA.

[dknopik]

Thanks for the PR!

Side comment:
As far as I can tell cargo test doesn't work on the stable branch, because in the top level Cargo.toml there is a sync feature missing in the tokio dependency import.

Yeah, I also noticed that the tests for the database can not be run on their own due to this. There is some other PR where I add the "sync" feature.

Removed NOT NULL on fee_recipient, had to do it because on the BUMP_NONCE command we are only inserting on owner and nonce.

Makes sense. But I think we might need to adjust this:

anchor/anchor/database/src/validator_operations.rs

Line 46 in e671f38

let address_str: String = row.get(0)?;

to also accept NULL values by reading an Option<String>.

I think defaulting the nonce to 0 might cause issues, as setting the fee recipient will set the nonce to 0 if there was no row before. This is different from an unset nonce, as a set nonce of 0 implies that nonce 0 was used already. I suspect this might cause issues with syncing for owners that set the recipient before registering any validators.

I will add two test cases to illustrate these issues :)

[petarjuki7]

I changed the in-memory storage a bit.
With previous in-memory storage we would assume the nonce is always there:
nonces: HashMap<Address, u16>

I changed nonce storage to HashMap<Address, Option<u16>> to properly represent:

• None = owner exists but has never used a nonce
• Some(0) = owner has used nonce 0
• Some(n) = owner has used nonce n

Also modified all the related nonce code to use Option<u16> instead of u16
I also updated BUMP_NONCE SQL to use COALESCE(nonce, -1) + 1 to handle NULL nonces

I'm happy changing it either way you see fit.

A simpler solution is to leave it like it was, and handle situations with NULL nonces like this:
let nonce: u64 = row.get::<_, Option<u64>>(0)?.unwrap_or(0);

[dknopik]

Oooh, good idea, I like it :)

I think COALESCE(nonce + 1, 0) might be easier to understand and should also work because NULL + 1 computes to NULL.

[mergify]

Some required checks have failed. Could you please take a look @petarjuki7? 🙏

[Copilot]

Pull Request Overview

Combines the previous owners and nonce tables into a single owners table, making both fee_recipient and nonce nullable and updating all related queries and code paths accordingly.

• Merged schema: replaced separate owners and nonce tables with one owners table containing both fields
• Refactored SQL operations and Rust code to handle nullable fee_recipient and nonce
• Added tests for NULL handling of fee_recipient and nonce consistency across operations

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
anchor/database/src/validator_operations.rs	Adapt fee_recipient_for_owner to return Option<Address>
anchor/database/src/tests/cluster_tests.rs	Add tests for NULL fee_recipient and nonce progression logic
anchor/database/src/table_schema.sql	Drop old tables and define unified owners schema
anchor/database/src/state.rs	Filter out rows with NULL nonce when collecting nonces
anchor/database/src/sql_operations.rs	Update SQL queries for merged owners table

[Copilot]

[nitpick] The filter_map call silently drops owners with a NULL nonce. If that’s intended, add a comment clarifying this behavior; otherwise consider handling the None case explicitly.

Suggested change

	})?
	})?
	// Owners with a `NULL` nonce are intentionally dropped here.

[diegomrsantos]

I changed the in-memory storage a bit. With previous in-memory storage we would assume the nonce is always there: nonces: HashMap<Address, u16>

I changed nonce storage to HashMap<Address, Option<u16>> to properly represent:

• None = owner exists but has never used a nonce
• Some(0) = owner has used nonce 0
• Some(n) = owner has used nonce n

Also modified all the related nonce code to use Option<u16> instead of u16 I also updated BUMP_NONCE SQL to use COALESCE(nonce, -1) + 1 to handle NULL nonces

I'm happy changing it either way you see fit.

A simpler solution is to leave it like it was, and handle situations with NULL nonces like this: let nonce: u64 = row.get::<_, Option<u64>>(0)?.unwrap_or(0);

Can we say an unused nonce is 0?

[petarjuki7]

Can we say an unused nonce is 0?

Since nonces in Ethereum start with 0 (the first transactions nonce is 0), then maybe putting it at 0 could read as "already had a first transaction as nonce 0" and not that it hasn't been set yet

[diegomrsantos]

Can we say an unused nonce is 0?

Since nonces in Ethereum start with 0 (the first transactions nonce is 0), then maybe putting it at 0 could read as "already had a first transaction as nonce 0" and not that it hasn't been set yet

Are you sure? The yellow paper defines nonce as the number of transactions sent from the address.

[diegomrsantos]

Can we say an unused nonce is 0?

Since nonces in Ethereum start with 0 (the first transactions nonce is 0), then maybe putting it at 0 could read as "already had a first transaction as nonce 0" and not that it hasn't been set yet

Are you sure? The yellow paper defines nonce as the number of transactions sent from the address.

It also says "An account is empty when it has no code, zero nonce and zero balance"

[dknopik]

We are not talking about Ethereum nonces here, but nonces as used in the SSV network.

In the database, we store the previous, not the next nonce. Therefore there is a difference between 0 (which means the last ValidatorAdded processed was supposed to have a nonce of 0) and NULL (which means that there was no ValidatorAdded message yet).

[dknopik]

To clear up the confusion why I put " 👍 " to

Since nonces in Ethereum start with 0 (the first transactions nonce is 0), then maybe putting it at 0 could read as "already had a first transaction as nonce 0" and not that it hasn't been set yet

While we do not work with Ethereum nonces, but with nonces as used in SSV's ValidatorAdded events, I liked that comment as it highlighted that we store the nonce of the already seen event, not of the expected nonce of the next event (which is also the current event count).

I will test this change on some nodes to ensure it's working fine, then we can merge this :) Thanks @petarjuki7!

[dknopik]

I found another small issue: we needed to account for the null value when fetch_clusters is run during startup. I fixed it myself, hope you don't mind :)

We will merge this soon after deciding what will go into the next version 👍

[petarjuki7]

We will merge this soon after deciding what will go into the next version 👍

Great, thanks! :)

Thought of something else that might be worth changing:
currently in the bump_and_get_nonce() function, we update the nonce manually in memory:

anchor/anchor/database/src/cluster_operations.rs

Lines 164 to 180 in 94d9d4a

	let mut nonce = 0;
	self.modify_state(|state| {
	// bump the nonce in memory
	if !state.single_state.nonces.contains_key(owner) {
	// if it does not yet exist in memory, then create an entry and set it to zero
	state.single_state.nonces.insert(*owner, 0);
	} else {
	// otherwise, just increment the entry
	let entry = state
	.single_state
	.nonces
	.get_mut(owner)
	.expect("This must exist");
	*entry += 1;
	nonce = *entry;
	}
	});

It could be changed to:

let mut stmt = tx.prepare_cached(sql_operations::GET_NONCE)?;
let nonce: u16 = stmt.query_row(params![owner.to_string()], |row| row.get(0))?;

// Update in memory with the truth from the database
self.modify_state(|state| {
    state.single_state.nonces.insert(*owner, nonce);
});
Ok(nonce)

So we are sure it is the same in the database as in memory. This removes any risk of the in-memory logic diverging from the SQL logic.

This passes all the tests also, let me know what do you think, I can include it in a commit.

[dknopik]

LGTM, thanks @petarjuki7!

[dknopik]

@petarjuki7

Thanks for your suggestion!

Our caching layer operates under the assumption that the cache update is implemented consistently with the actual database upgrade. Also, no other processes modify the database - which will be enforced by #358. So I think to keep the approach to DB updates consistent, we should keep the current state.

@ThreeHrSleep maybe this is a good target for fuzz testing to ensure the update functions are correct in all cases:

• create some input events and apply them
• capture the state
• reopen database
• check if the reopened state matches