Append client tag to graffiti _by default_

[michaelsproul]

Description

We should enable the --graffiti-append flag from this PR by default:

• Append client version info to graffiti #7558

Because we've already released it, we need to do this in a backwards-compatible way. I think something like making the flag accept Option<bool> would work well, i.e. all of these would be accepted:

• --graffiti-append true: do append
• --graffiti-append false: don't append
• --graffiti-append: do append
• <nothing>: do append

We should have CLI tests for each of these options as well.

[chong-he]

I didn't enable by default back then, as I thought we want to let users to opt-in (maybe some users don't prefer to have their graffiti changed)

But to have it on by default, and let users opt-out is a good idea. I will look into this

Also, a bit of semantic, the description of the flag mentions "prepend" as I thought it is more correct (added before instead of after), but I forgot to change the flag name back then:

 --graffiti-append
          When used, client version info will be prepended to user custom
          graffiti, with a space in between. This should only be used with a
          Lighthouse beacon node.

We can probably change the flag name too?

[michaelsproul]

Oh 😅 Changing the flag name is a breaking change.

Why do we prepend? I think it looks like Teku are appending at the end?

https://docs.teku.consensys.io/reference/cli#validators-graffiti-client-append-format

[chong-he]

oh right, probably should have keep the flag name.

I follow Mark's note on this: hackmd.io/@wmoBhF17RAOH2NZ5bNXJVg/BJX2c9gja

This is also in the original issue #7201

[chong-he]

I recall the reason why we didn't make graffiti_append a default back then.

We modify the get_validator_blocks_v3 path with graffiti policy as an input in the function:

lighthouse/common/eth2/src/lib.rs

Lines 2203 to 2247 in 176cce5

	pub async fn get_validator_blocks_v3_path(
	&self,
	slot: Slot,
	randao_reveal: &SignatureBytes,
	graffiti: Option<&Graffiti>,
	skip_randao_verification: SkipRandaoVerification,
	builder_booster_factor: Option<u64>,
	graffiti_policy: Option<GraffitiPolicy>,
	) -> Result<Url, Error> {
	let mut path = self.eth_path(V3)?;
	path.path_segments_mut()
	.map_err(|()| Error::InvalidUrl(self.server.clone()))?
	.push("validator")
	.push("blocks")
	.push(&slot.to_string());
	path.query_pairs_mut()
	.append_pair("randao_reveal", &randao_reveal.to_string());
	if let Some(graffiti) = graffiti {
	path.query_pairs_mut()
	.append_pair("graffiti", &graffiti.to_string());
	}
	if skip_randao_verification == SkipRandaoVerification::Yes {
	path.query_pairs_mut()
	.append_pair("skip_randao_verification", "");
	}
	if let Some(builder_booster_factor) = builder_booster_factor {
	path.query_pairs_mut()
	.append_pair("builder_boost_factor", &builder_booster_factor.to_string());
	}
	// Only append the HTTP URL request if the graffiti_policy is to AppendClientVersions
	// If PreserveUserGraffiti (default), then the HTTP URL request does not contain graffiti_policy
	// so that the default case is compliant to the spec
	if let Some(GraffitiPolicy::AppendClientVersions) = graffiti_policy {
	path.query_pairs_mut()
	.append_pair("graffiti_policy", "AppendClientVersions");
	}
	Ok(path)
	}

We want to make sure that by default, we don't modify the path of the http query, so the default is GraffitiPolicy::PreserveUserGraffiti. We have a test here to make sure this:

lighthouse/beacon_node/http_api/tests/tests.rs

Lines 6878 to 6953 in 176cce5

	async fn get_validator_blocks_v3_path_graffiti_policy(self) -> Self {
	let slot = self.chain.slot().unwrap();
	let epoch = self.chain.epoch().unwrap();
	let (_, randao_reveal) = self.get_test_randao(slot, epoch).await;
	let graffiti = Some(Graffiti::from([0; GRAFFITI_BYTES_LEN]));
	let builder_boost_factor = None;
	// Default case where GraffitiPolicy is None
	let default_path = self
	.client
	.get_validator_blocks_v3_path(
	slot,
	&randao_reveal,
	graffiti.as_ref(),
	SkipRandaoVerification::Yes,
	builder_boost_factor,
	None,
	)
	.await
	.unwrap();
	let query_default_path = default_path.query().unwrap_or("");
	// When GraffitiPolicy is None, the HTTP API query path should not contain "graffiti_policy"
	assert!(
	!query_default_path.contains("graffiti_policy"),
	"URL should not contain graffiti_policy parameter (same as PreserveUserGraffiti). URL is: {}",
	query_default_path
	);
	let preserve_path = self
	.client
	.get_validator_blocks_v3_path(
	slot,
	&randao_reveal,
	graffiti.as_ref(),
	SkipRandaoVerification::Yes,
	builder_boost_factor,
	Some(GraffitiPolicy::PreserveUserGraffiti),
	)
	.await
	.unwrap();
	let query_preserve_path = preserve_path.query().unwrap_or("");
	// When GraffitiPolicy is set to PreserveUserGraffiti, the HTTP API query path should not contain "graffiti_policy"
	assert!(
	!query_preserve_path.contains("graffiti_policy"),
	"URL should not contain graffiti_policy parameter when using PreserveUserGraffiti. URL is: {}",
	query_preserve_path
	);
	// The HTTP API query path for PreserveUserGraffiti should be the same as the default
	assert_eq!(query_default_path, query_preserve_path);
	let append_path = self
	.client
	.get_validator_blocks_v3_path(
	slot,
	&randao_reveal,
	graffiti.as_ref(),
	SkipRandaoVerification::No,
	builder_boost_factor,
	Some(GraffitiPolicy::AppendClientVersions),
	)
	.await
	.unwrap();
	let query_append_path = append_path.query().unwrap_or("");
	// When GraffitiPolicy is AppendClientVersions, the HTTP API query path should contain "graffiti_policy"
	assert!(
	query_append_path.contains("graffiti_policy"),
	"URL should contain graffiti_policy=AppendClientVersions parameter. URL is: {}",
	query_append_path
	);
	self
	}
	}

If we change the default, it would then change the http query path. Is this ok?

[michaelsproul]

Ohhh right. I think we can get away with flipping the defaults?

So rather than sending ?graffiti_policy=AppendClientVersions if the config says to append, and nothing for preserve, we switch to:

• no parameter (default, if graffiti_policy is append)
• ?graffiti_policy=PreserveUserGraffiti (if graffiti_policy is preserve)

[michaelsproul]

This will be a small breaking change though (it will change the behaviour of LH v8.1.x VC with LH v8.2.0 BN)

[chong-he]

Closed by #9313