chore: move clarity-cli to contrib

[Jiloc]

Description

This PR builds on top of #6532 and will stay in draft until that one will be merged.

Applicable issues

• fixes [chore] move helper binaries to contrib folder #6493

Additional info (benefits, drawbacks, caveats)

Checklist

• Test coverage for new or modified code paths
• Changelog is updated
• Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
• New clarity functions have corresponding PR in clarity-benchmarking repo
• New integration test(s) added to bitcoin-tests.yml

[jcnelson]

I notice that the integration tests use clarity_cli. To me, this means that clarity_cli does not belong in contrib/, since that's where we put software that has no definitive level of support.

• Do we intend to drop or limit support for clarity_cli, and thereby justify its removal to contrib/? I personally find myself using it on an almost daily basis, so I'm happy to be the code owner

• Is there a way to separate out the vm_execute() function from clarity_cli that the integration tests need?

[Jiloc]

I notice that the integration tests use clarity_cli. To me, this means that clarity_cli does not belong in contrib/, since that's where we put software that has no definitive level of support.

• Do we intend to drop or limit support for clarity_cli, and thereby justify its removal to contrib/? I personally find myself using it on an almost daily basis, so I'm happy to be the code owner
• Is there a way to separate out the vm_execute() function from clarity_cli that the integration tests need?

Thanks for bringing this up! The move to contrib/ is purely for bug bounty scope reduction. We discovered CLI utilities (e.g. stacks-events, that was even broken!) were inadvertently included in our ImmuneFi bounty as part of stackslib. No intention to drop support for clarity-cli at all, just to move it to a less critical place.

Regarding the dependency, I checked it and vm_execute was just almost exaclty a duplication of execute_with_parameters_and_call_in_global_context from clarity. Probably it was duplicated because the original function is defined behind the "testing" feature flag. I refactored the code in the stacks-core tests to remove clarity-cli as a dependency and to directly use the function from clarity.

stacks-core/clarity/src/vm/mod.rs

Lines 517 to 566 in 895537e

	#[cfg(any(test, feature = "testing"))]
	pub fn execute_with_parameters_and_call_in_global_context<F>(
	program: &str,
	clarity_version: ClarityVersion,
	epoch: StacksEpochId,
	use_mainnet: bool,
	mut global_context_function: F,
	) -> Result<Option<Value>>
	where
	F: FnMut(&mut GlobalContext) -> Result<()>,
	{
	use crate::vm::database::MemoryBackingStore;
	use crate::vm::tests::test_only_mainnet_to_chain_id;
	use crate::vm::types::QualifiedContractIdentifier;
	let contract_id = QualifiedContractIdentifier::transient();
	let mut contract_context = ContractContext::new(contract_id.clone(), clarity_version);
	let mut marf = MemoryBackingStore::new();
	let conn = marf.as_clarity_db();
	let chain_id = test_only_mainnet_to_chain_id(use_mainnet);
	let mut global_context = GlobalContext::new(
	use_mainnet,
	chain_id,
	conn,
	LimitedCostTracker::new_free(),
	epoch,
	);
	global_context.execute(|g| {
	global_context_function(g)?;
	let parsed =
	ast::build_ast(&contract_id, program, &mut (), clarity_version, epoch)?.expressions;
	eval_all(&parsed, &mut contract_context, g, None)
	})
	}
	#[cfg(any(test, feature = "testing"))]
	pub fn execute_with_parameters(
	program: &str,
	clarity_version: ClarityVersion,
	epoch: StacksEpochId,
	use_mainnet: bool,
	) -> Result<Option<Value>> {
	execute_with_parameters_and_call_in_global_context(
	program,
	clarity_version,
	epoch,
	use_mainnet,
	|_| Ok(()),
	)
	}

Does this approach make sense, or do you have a better idea?

[jferrant]

LGTM

[federico-stacks]

Reviving this PR with #6670

[francesco-stacks]

LGTM!

[jacinta-stacks]

Looks good, I know a lot of those failing tests are failing on develop, but this does seem to be a bit more than normal. Maybe just try running the tests on develop and on this branch to confirm if they are newly flakey due to this PR or not.

[simone-stacks]

After re-running the tests, I can confirm that some of them passed and that we reduced the number of failing ones:

• Nakamoto
  • tests::nakamoto_integrations::correct_burn_outs
    • Failed also in attempt 2 and 3 ❌
  • tests::nakamoto_integrations::simple_neon_integration
    • Failed also in attempt 2 and 3 ❌
  • tests::nakamoto_integrations::smaller_tenure_size_for_miner_with_tenure_extend
    • Failed also in attempt 2 and 3 ❌
  • tests::nakamoto_integrations::test_sip_031_last_phase_coinbase_matches_activation
    • Failed also in attempt 2 and 3 ❌
• Signer
  • tests::signer::v0::allow_reorg_within_first_proposal_burn_block_timing_secs_scenario
    • Worked in attempt 2 ✅
  • tests::signer::v0::locally_rejected_blocks_overriden_by_global_acceptance
    • Worked in attempt 2 ✅
  • tests::signer::v0::mine_2_nakamoto_reward_cycles
    • Failed also in attempt 2 and 3 ❌
  • tests::signer::v0::multiple_miners_with_custom_chain_id
    • Worked in attempt 2 ✅
  • tests::signer::v0::multiple_miners_with_nakamoto_blocks
    • Worked in attempt 2 ✅

What do you think about it?

[jacinta-stacks]

After re-running the tests, I can confirm that some of them passed and that we reduced the number of failing ones:

* Nakamoto
  
  * tests::nakamoto_integrations::correct_burn_outs
    
    * Failed also in attempt 2 and 3 ❌
  * tests::nakamoto_integrations::simple_neon_integration
    
    * Failed also in attempt 2 and 3 ❌
  * tests::nakamoto_integrations::smaller_tenure_size_for_miner_with_tenure_extend
    
    * Failed also in attempt 2 and 3 ❌
  * tests::nakamoto_integrations::test_sip_031_last_phase_coinbase_matches_activation
    
    * Failed also in attempt 2 and 3 ❌

* Signer
  
  * tests::signer::v0::allow_reorg_within_first_proposal_burn_block_timing_secs_scenario
    
    * Worked in attempt 2 ✅
  * tests::signer::v0::locally_rejected_blocks_overriden_by_global_acceptance
    
    * Worked in attempt 2 ✅
  * tests::signer::v0::mine_2_nakamoto_reward_cycles
    
    * Failed also in attempt 2 and 3 ❌
  * tests::signer::v0::multiple_miners_with_custom_chain_id
    
    * Worked in attempt 2 ✅
  * tests::signer::v0::multiple_miners_with_nakamoto_blocks
    
    * Worked in attempt 2 ✅

What do you think about it?

I don't think they have to do with your change. Especially since this is just moving code around. I would merge. :)