Merge remote-tracking branch 'raymondkfcheung/ray-align-storage-period' into ray-align-common-fn

Author: bkontur

.github/workflows/integration-test.yml

@@ -26,6 +26,12 @@ jobs:
     timeout-minutes: 60
 
     steps:
+      - name: Free Disk Space (Ubuntu)
+        if: ${{ runner.environment == 'github-hosted' }}
+        uses: jlumbroso/free-disk-space@54081f138730dfa15788a46383842cd2f914a1be # v1.3.1
+        with:
+          tool-cache: false
+
       - name: Checkout sources
         uses: actions/checkout@v4
 
@@ -65,14 +71,32 @@ jobs:
           chmod +x zombienet-linux-x64
           echo "ZOMBIENET_BINARY=$GITHUB_WORKSPACE/zombienet-linux-x64" >> $GITHUB_ENV
 
-      - name: Run authorize and store (PAPI, smoldot)
+      - name: Run authorize and store (PAPI, RPC node)
         working-directory: examples
-        run: just run-authorize-and-store "smoldot"
+        run: |
+          export TEST_DIR="$(mktemp -d $GITHUB_WORKSPACE/bulletin-tests-run-XXXXX)/test"
+          echo "TEST_DIR=$TEST_DIR" >> $GITHUB_ENV
+          just run-authorize-and-store "ws"
 
-      - name: Run authorize and store (PAPI, RPC node)
+      - name: Run authorize and store (PAPI, smoldot)
         working-directory: examples
-        run: just run-authorize-and-store "ws"
+        run: |
+          export TEST_DIR="$(mktemp -d $GITHUB_WORKSPACE/bulletin-tests-run-XXXXX)/test"
+          echo "TEST_DIR=$TEST_DIR" >> $GITHUB_ENV
+          just run-authorize-and-store "smoldot"
 
       - name: Run store chunked data + DAG-PB (PJS-API, RPC node)
         working-directory: examples
-        run: just run-store-chunked-data
+        run: |
+          export TEST_DIR="$(mktemp -d $GITHUB_WORKSPACE/bulletin-tests-run-XXXXX)/test"
+          echo "TEST_DIR=$TEST_DIR" >> $GITHUB_ENV
+          just run-store-chunked-data
+
+      # Collects logs from the last failed zombienet run.
+      - name: Upload Zombienet logs (on failure)
+        if: failure()
+        uses: actions/upload-artifact@v4
+        with:
+          name: failed-zombienet-logs
+          path: |
+            ${{ env.TEST_DIR }}/*.log

examples/justfile

@@ -50,14 +50,19 @@ bulletin-solo-zombienet-start:
         echo "   Set ZOMBIENET_BINARY environment variable or ensure zombienet-* exists in parent directory"
         exit 1
     fi
-    
-    echo "   Using zombienet: $ZOMBIENET_BIN"
-    
-    # Get absolute paths for bulletin binary and config
+
+    # Get absolute paths, base dir, bulletin binary and config
+    if [ -n "$TEST_DIR" ]; then
+        TEST_DIR="$TEST_DIR"
+    else
+        # TEST_DIR is not set, create a new temp directory
+        TEST_DIR="$(mktemp -d /tmp/bulletin-tests-run-XXXXX)/test"
+    fi
     BULLETIN_BINARY=$(cd .. && pwd)/target/release/polkadot-bulletin-chain
     ZOMBIENET_CONFIG=$(cd .. && pwd)/zombienet/bulletin-polkadot-local.toml
-    
-    POLKADOT_BULLETIN_BINARY_PATH=$BULLETIN_BINARY $ZOMBIENET_BIN -p native spawn $ZOMBIENET_CONFIG > /tmp/zombienet.log 2>&1 &
+    echo "   Using zombienet: $ZOMBIENET_BIN, working directory: $TEST_DIR, zombienet config: $ZOMBIENET_CONFIG"
+
+    POLKADOT_BULLETIN_BINARY_PATH=$BULLETIN_BINARY $ZOMBIENET_BIN --dir $TEST_DIR -p native spawn $ZOMBIENET_CONFIG > /tmp/zombienet.log 2>&1 &
     ZOMBIENET_PID=$!
     echo $ZOMBIENET_PID > {{ PID_DIR }}/zombienet.pid
     echo "   Zombienet PID: $ZOMBIENET_PID"
@@ -185,9 +190,7 @@ setup-services:
     sleep 10
     echo "   Zombienet process check:"
     ps aux | grep zombienet | grep -v grep || echo "   ⚠ No zombienet process found"
-    echo "   Checking zombienet log:"
-    tail -20 /tmp/zombienet.log || echo "   ⚠ Could not read zombienet log"
-    
+
     just ipfs-connect
     just ipfs-reconnect-start
     just papi-generate

pallets/transaction-storage/README.md

@@ -2,8 +2,8 @@
 
 Indexes transactions and manages storage proofs.
 
-Allows storing arbitrary data on the chain. Data is automatically removed after `StoragePeriod` blocks, unless the storage is renewed.
-Validators must submit proof of storing a random chunk of data for block `N - StoragePeriod` when producing block `N`.
+Allows storing arbitrary data on the chain. Data is automatically removed after `RetentionPeriod` blocks, unless the storage is renewed.
+Validators must submit proof of storing a random chunk of data for block `N - RetentionPeriod` when producing block `N`.
 
 # Running a chain
 
@@ -16,7 +16,7 @@ cargo run --release -- build-spec --chain=local > sc_init.json
 ```
 
 Edit the json chain spec file to customise the chain. The storage chain genesis params are configured in the `transactionStorage` section.
-Note that `storagePeriod` is specified in blocks and changing it also requires code changes at the moment.
+Note that `retentionPeriod` is specified in blocks and changing it also requires code changes at the moment.
 
 Build a raw spec from the init spec.
 
@@ -32,7 +32,7 @@ cargo run --release -- --chain=sc.json -d /tmp/bob --storage-chain --keep-blocks
 ```
 
 `--storage-chain` enables transaction indexing.
-`--keep-blocks=100800` enables block pruning. The value here should be greater or equal than the storage period.
+`--keep-blocks=100800` enables block pruning. The value here should be greater or equal than the retention period.
 `--ipfs-server` enables serving stored content over IPFS.
 
 Once the network is started, any other joining nodes need to sync with `--sync=fast`. Regular sync will fail because block pruning removes old blocks. The chain does not keep full block history.
@@ -75,7 +75,7 @@ ipfs swarm connect <substrate peer address>
 ipfs block get /ipfs/<CID> > kitten.jpeg
 ```
 
-To renew data and prevent it from being disposed after the storage period, use `transactionStorage.renew(block, index)`
+To renew data and prevent it from being disposed after the retention period, use `transactionStorage.renew(block, index)`
 where `block` is the block number of the previous store or renew transction, and index is the index of that transaction in the block.
 
 

pallets/transaction-storage/src/benchmarking.rs

@@ -157,7 +157,7 @@ mod benchmarks {
 				vec![0u8; T::MaxTransactionSize::get() as usize],
 			)?;
 		}
-		run_to_block::<T>(StoragePeriod::<T>::get() + BlockNumberFor::<T>::one());
+		run_to_block::<T>(crate::Pallet::<T>::retention_period() + BlockNumberFor::<T>::one());
 		let encoded_proof = proof();
 		let proof = TransactionStorageProof::decode(&mut &*encoded_proof).unwrap();
 

pallets/transaction-storage/src/lib.rs

@@ -24,9 +24,12 @@
 // Ensure we're `no_std` when compiling for Wasm.
 #![cfg_attr(not(feature = "std"), no_std)]
 
+extern crate alloc;
+
 mod benchmarking;
 pub mod weights;
 
+pub mod migrations;
 #[cfg(test)]
 mod mock;
 #[cfg(test)]
@@ -37,7 +40,10 @@ use core::fmt::Debug;
 use polkadot_sdk_frame::{
 	deps::{sp_core::sp_std::prelude::*, *},
 	prelude::*,
-	traits::fungible::{Balanced, Credit, Inspect, Mutate, MutateHold},
+	traits::{
+		fungible::{Balanced, Credit, Inspect, Mutate, MutateHold},
+		parameter_types,
+	},
 };
 use sp_transaction_storage_proof::{
 	encode_index, num_chunks, random_chunk, ChunkIndex, InherentError, TransactionStorageProof,
@@ -55,6 +61,12 @@ pub use weights::WeightInfo;
 
 const LOG_TARGET: &str = "runtime::transaction-storage";
 
+/// Default retention period for data (in blocks).
+pub const DEFAULT_RETENTION_PERIOD: u32 = 100800;
+parameter_types! {
+	pub const DefaultRetentionPeriod: u32 = DEFAULT_RETENTION_PERIOD;
+}
+
 // TODO: https://github.com/paritytech/polkadot-bulletin-chain/issues/139 - Clarify purpose of allocator limits and decide whether to remove or use these constants.
 /// Maximum bytes that can be stored in one transaction.
 // Setting higher limit also requires raising the allocator limit.
@@ -253,7 +265,7 @@ pub mod pallet {
 			// Drop obsolete roots. The proof for `obsolete` will be checked later
 			// in this block, so we drop `obsolete` - 1.
 			weight.saturating_accrue(db_weight.reads(1));
-			let period = StoragePeriod::<T>::get();
+			let period = Self::retention_period();
 			let obsolete = n.saturating_sub(period.saturating_add(One::one()));
 			if obsolete > Zero::zero() {
 				weight.saturating_accrue(db_weight.writes(2));
@@ -271,7 +283,7 @@ pub mod pallet {
 				<ProofChecked<T>>::take() || {
 					// Proof is not required for early or empty blocks.
 					let number = <frame_system::Pallet<T>>::block_number();
-					let period = StoragePeriod::<T>::get();
+					let period = Self::retention_period();
 					let target_number = number.saturating_sub(period);
 
 					target_number.is_zero() || {
@@ -300,11 +312,11 @@ pub mod pallet {
 				!T::MaxTransactionSize::get().is_zero(),
 				"MaxTransactionSize must be greater than zero"
 			);
-			let default_period = sp_transaction_storage_proof::DEFAULT_STORAGE_PERIOD.into();
-			let storage_period = GenesisConfig::<T>::default().storage_period;
+			let default_period = DEFAULT_RETENTION_PERIOD.into();
+			let retention_period = GenesisConfig::<T>::default().retention_period;
 			assert_eq!(
-				storage_period, default_period,
-				"GenesisConfig.storage_period must match DEFAULT_STORAGE_PERIOD"
+				retention_period, default_period,
+				"GenesisConfig.retention_period must match DEFAULT_RETENTION_PERIOD"
 			);
 			assert!(
 				!T::AuthorizationPeriod::get().is_zero(),
@@ -316,8 +328,8 @@ pub mod pallet {
 	#[pallet::call]
 	impl<T: Config> Pallet<T> {
 		/// Index and store data off chain. Minimum data size is 1 byte, maximum is
-		/// `MaxTransactionSize`. Data will be removed after `StoragePeriod` blocks, unless `renew`
-		/// is called.
+		/// `MaxTransactionSize`. Data will be removed after `RetentionPeriod` blocks, unless
+		/// `renew` is called.
 		///
 		/// Authorization is required to store data using regular signed/unsigned transactions.
 		/// Regular signed transactions require account authorization (see
@@ -420,7 +432,7 @@ pub mod pallet {
 			Ok(().into())
 		}
 
-		/// Check storage proof for block number `block_number() - StoragePeriod`. If such a block
+		/// Check storage proof for block number `block_number() - RetentionPeriod`. If such a block
 		/// does not exist, the proof is expected to be `None`.
 		///
 		/// ## Complexity
@@ -438,7 +450,7 @@ pub mod pallet {
 
 			// Get the target block metadata.
 			let number = <frame_system::Pallet<T>>::block_number();
-			let period = StoragePeriod::<T>::get();
+			let period = Self::retention_period();
 			let target_number = number.saturating_sub(period);
 			ensure!(!target_number.is_zero(), Error::<T>::UnexpectedProof);
 			let transactions =
@@ -641,10 +653,13 @@ pub mod pallet {
 	/// Storage fee per transaction.
 	pub type EntryFee<T: Config> = StorageValue<_, BalanceOf<T>>;
 
-	/// Storage period for data in blocks. Should match `sp_storage_proof::DEFAULT_STORAGE_PERIOD`
-	/// for block authoring.
+	/// Number of blocks for which stored data must be retained.
+	///
+	/// Data older than `RetentionPeriod` blocks is eligible for removal unless it
+	/// has been explicitly renewed. Validators are required to prove possession of
+	/// data corresponding to block `N - RetentionPeriod` when producing block `N`.
 	#[pallet::storage]
-	pub type StoragePeriod<T: Config> = StorageValue<_, BlockNumberFor<T>, ValueQuery>;
+	pub type RetentionPeriod<T: Config> = StorageValue<_, BlockNumberFor<T>, ValueQuery>;
 
 	// Intermediates
 	#[pallet::storage]
@@ -659,15 +674,15 @@ pub mod pallet {
 	pub struct GenesisConfig<T: Config> {
 		pub byte_fee: BalanceOf<T>,
 		pub entry_fee: BalanceOf<T>,
-		pub storage_period: BlockNumberFor<T>,
+		pub retention_period: BlockNumberFor<T>,
 	}
 
 	impl<T: Config> Default for GenesisConfig<T> {
 		fn default() -> Self {
 			Self {
 				byte_fee: 10u32.into(),
 				entry_fee: 1000u32.into(),
-				storage_period: sp_transaction_storage_proof::DEFAULT_STORAGE_PERIOD.into(),
+				retention_period: DEFAULT_RETENTION_PERIOD.into(),
 			}
 		}
 	}
@@ -677,7 +692,7 @@ pub mod pallet {
 		fn build(&self) {
 			ByteFee::<T>::put(self.byte_fee);
 			EntryFee::<T>::put(self.entry_fee);
-			StoragePeriod::<T>::put(self.storage_period);
+			RetentionPeriod::<T>::put(self.retention_period);
 		}
 	}
 
@@ -868,6 +883,11 @@ pub mod pallet {
 			Self::check_signed(who, call, CheckContext::PreDispatch).map(|_| ())
 		}
 
+		/// Get RetentionPeriod storage information from the outside of this pallet.
+		pub fn retention_period() -> BlockNumberFor<T> {
+			RetentionPeriod::<T>::get()
+		}
+
 		/// Returns `true` if a blob of the given size can be stored.
 		fn data_size_ok(size: usize) -> bool {
 			(size > 0) && (size <= T::MaxTransactionSize::get() as usize)

pallets/transaction-storage/src/migrations.rs

@@ -0,0 +1,61 @@
+// Copyright (C) Parity Technologies (UK) Ltd.
+// SPDX-License-Identifier: Apache-2.0
+
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// 	http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+use crate::{Config, RetentionPeriod, LOG_TARGET};
+use core::marker::PhantomData;
+use polkadot_sdk_frame::{
+	prelude::{BlockNumberFor, Weight},
+	traits::{Get, OnRuntimeUpgrade, Zero},
+};
+
+/// Runtime migration that sets the `RetentionPeriod` storage item to a
+/// non-zero `NewValue` value **only if it is currently zero**.
+///
+/// Idempotent migration: safe to run multiple times
+pub struct SetRetentionPeriodIfZero<T, NewValue>(PhantomData<(T, NewValue)>);
+impl<T: Config, NewValue: Get<BlockNumberFor<T>>> OnRuntimeUpgrade
+	for SetRetentionPeriodIfZero<T, NewValue>
+{
+	fn on_runtime_upgrade() -> Weight {
+		let mut weight = T::DbWeight::get().reads(1);
+
+		// If zero, let's reset.
+		if RetentionPeriod::<T>::get().is_zero() {
+			RetentionPeriod::<T>::set(NewValue::get());
+			weight.saturating_accrue(T::DbWeight::get().writes(1));
+
+			tracing::warn!(
+				target: LOG_TARGET,
+				new_value = ?NewValue::get(),
+				"[SetRetentionPeriodIfZero] RetentionPeriod was zero, resetting to:",
+			);
+		}
+
+		weight
+	}
+
+	#[cfg(feature = "try-runtime")]
+	fn post_upgrade(
+		_state: alloc::vec::Vec<u8>,
+	) -> Result<(), polkadot_sdk_frame::deps::sp_runtime::DispatchError> {
+		polkadot_sdk_frame::prelude::ensure!(
+			!RetentionPeriod::<T>::get().is_zero(),
+			"must be migrate to the `NewValue`."
+		);
+
+		tracing::info!(target: LOG_TARGET, "SetRetentionPeriodIfZero is OK!");
+		Ok(())
+	}
+}

pallets/transaction-storage/src/mock.rs

@@ -71,7 +71,7 @@ pub fn new_test_ext() -> TestExternalities {
 	let t = RuntimeGenesisConfig {
 		system: Default::default(),
 		transaction_storage: pallet_transaction_storage::GenesisConfig::<Test> {
-			storage_period: 10,
+			retention_period: 10,
 			byte_fee: 2,
 			entry_fee: 200,
 		},