Data further refactor #2574

dirvine · 2024-12-24T22:54:40Z

wip - this PR is on top of the data_refactor PR

grumbach · 2024-12-30T15:10:22Z

ant-bootstrap/tests/cli_integration_tests.rs

+fn test_cli_add_peer() -> Result<()> {
+    let temp_dir = TempDir::new()?;
+    let cache_path = temp_dir.path().join("cache.txt");
+
+    let peer_addr = format!(
+        "/ip4/{}/udp/8080/quic-v1/p2p/12D3KooWRBhwfeP2Y4TCx1SM6s9rUoHhR5STiGwxBhgFRcw3UERE",
+        LOCAL_IP
+    );
+
+    // ... rest of the test ...
+    Ok(())
+}


Agreed that this would be a nice feature! Maybe add a TODO comment in the tests here so we don't forget them as is?

grumbach · 2024-12-30T15:14:51Z

ant-evm/src/amount.rs

    /// Converts the Nanos into bytes
    pub fn to_bytes(&self) -> Vec<u8> {


Here the comment still refers to Nanos

Yea I missed that one. This file was failing tests mind you, that's the only reason I ended up making changes to it.

grumbach · 2024-12-30T15:18:33Z

ant-evm/src/amount.rs

@@ -67,7 +67,7 @@ impl AttoTokens {

    /// Converts the Nanos into bytes
    pub fn to_bytes(&self) -> Vec<u8> {
-        self.0.as_le_bytes().to_vec()
+        self.0.to_be_bytes::<32>().to_vec()


Any specific reasons to move to big endian and 32 here?

Really network type endianness (most significant bit first) The 32 states we want exactly 32bytes (256 bits)
Otherwise we risk returning a variable length vec, well we would if using different types at least. So here we force the 32bytes.

In short we guarantee a 32byte network endian number here really.

grumbach · 2024-12-30T15:23:59Z

ant-networking/Cargo.toml

@@ -58,7 +59,6 @@ prometheus-client = { version = "0.22", optional = true }
 rand = { version = "~0.8.5", features = ["small_rng"] }
 rayon = "1.8.0"
 rmp-serde = "1.1.1"
-self_encryption = "~0.30.0"


Glad we were able to trim dependencies here!

grumbach · 2024-12-30T15:25:08Z

ant-networking/src/driver.rs

+    pub fn local(mut self, local: bool) -> Self {
+        self.local = local;
+        self
+    }
+


This is a GREAT pattern!

grumbach · 2024-12-30T18:58:24Z

autonomi/README_WASM.md

-# Autonomi JS API
-
-Note: the JS API is experimental and will be subject to change.


Are we sure we want to delete these WASM docs now?

yes we gave up on wasm for now. I hope we introduce it at a later stage, but we need to ensure we do not hamper network or disk access when we do

grumbach · 2024-12-30T20:06:46Z

autonomi/src/client/data/public.rs

+        match &self.mode {
+            ClientMode::ReadWrite(_) => {
+                let now = ant_networking::target_arch::Instant::now();


Why introduce a mode here?

The idea is we have a read only client (no wallet etc.) and a full client that can put so needs a wallet

* Add new Pointer type for network addressing * Add PointerAddress type and related network address handling * Implement signature verification for Pointer records * Add validation and storage handling for Pointer records in ant-node * Add log markers for Pointer operations * Make LinkedList outputs field optional by wrapping in Option<Vec> * Update LinkedList constructors and signature calculation to handle optional outputs * Fix transaction tests to use .into() for empty output vectors * Update ant-networking to handle split records for both Pointer and optional outputs * Maintain signature verification and count-based selection for split records * Add InvalidSignature error variant to handle Pointer validation failures This change introduces a new Pointer type for network addressing while also making LinkedList transactions more flexible by allowing them to be created without outputs. The networking layer has been updated to handle both changes, including proper split record handling, signature verification, and storage validation. All changes maintain backward compatibility with existing functionality.

…version numbers, and add tests

…apLevel with direct streaming API usage - Update client utils to use streaming_decrypt_from_storage - Add detailed implementation and schedule documentation - Clean up unused imports and improve error handling - Update archive.rs to work with new self_encryption API

…pe - Remove WASM test file and documentation - Update README to remove WASM references

…cal mode instead of CLOSE_GROUP_SIZE - Remove global address check for local testing - Fix test configuration to properly use local feature - Clean up unused imports and variables

- Refactor LocalNode to use tokio async/await consistently - Add proper timeouts and cleanup in start/stop methods - Improve process handling with kill_on_drop - Remove unused peer discovery and event handling - Clean up imports and remove warnings - Add comprehensive test with proper cleanup

…very - Replace ipfs with antnode binary for node operations - Implement proper mDNS peer discovery - Add proper error handling and logging - Increase test timeouts for node startup - Fix compilation issues and dependencies

- Remove local feature flag dependency, making it a runtime configuration - Ensure bootstrap cache is not written to in local mode - Fix builder pattern usage in NetworkBuilder - Add proper config handling in deprecated connect method - Update tests to work with runtime local mode configuration This change makes the local mode behavior more explicit and configurable at runtime, rather than being a compile-time feature flag. It also ensures that when in local mode, we don't pollute the bootstrap cache with local-only peers.

- Update find_local_ip to use UDP sockets for QUIC protocol\n- Add comprehensive IP validation (non-loopback, private range, etc.)\n- Update multiaddr format to use QUIC instead of TCP\n- Fix error handling in network code and tests\n- Add test coverage for IP validation and QUIC multiaddr format\n\nThis change ensures we use the correct network interface for QUIC connections and validates the IP address meets all requirements for P2P networking.

…rap cache correctly

- Fix command-line argument order for antnode, placing global flags before subcommand - Add stdout/stderr capture to LocalNode for better debugging - Split node startup into first and secondary node functions - Add node status checking to detect early failures - Reduce test timeout from 120s to 30s for faster feedback - Add better logging of node addresses and connection status The test now properly starts two nodes in a local network configuration, with the first node having the --first flag and the second node joining as a peer. Output capturing helps diagnose startup and connection issues.

This commit enhances error handling and test infrastructure across the codebase: Error Handling: - Add Serialization variant to CostError for consistent error handling - Fix error conversions for serialization errors in archive handling - Ensure proper error propagation in client operations Module Organization: - Fix Client import in pointer.rs to use super::Client - Re-export PutError in lib.rs for test accessibility - Clean up import paths for better code organization Method Improvements: - Make check_write_access public for testing purposes - Fix wallet() method access to use proper method call Test Infrastructure: - Add local EVM network configuration for testing - Create EVM testnet data CSV file with test configuration - Fix test environment setup for client mode tests This change improves code maintainability and test reliability while ensuring consistent error handling across the codebase.

- Add tempfile dependency for temporary directory management - Create setup_test_environment function to handle test environment setup - Use temporary directory for EVM testnet data and configuration - Clean up imports and improve code organization

- Added utils module to ant-bootstrap with find_local_ip function - Removed ant-networking dependency from ant-bootstrap to avoid circular dependency - Updated address_format_tests to use the new utils function - Cleaned up unused imports

- Add test-local.sh script for running local tests - Add GitHub Actions workflow for running local tests - Add linked_list tests - Update client implementation for linked list operations - Clean up unused imports and fix linter warnings

- Add cleanup before starting EVM testnet - Set specific RPC port for EVM testnet

- Add port checks before starting EVM testnet - Use consistent port 4343 across scripts - Only clean up antnode processes, preserve EVM network

…hunk store implementations

…usage instructions

github-advanced-security

devskim found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

grumbach · 2024-12-30T20:13:52Z

autonomi/src/client/error.rs

+/// Errors that can occur during data storage operations
+#[derive(Debug, thiserror::Error)]
+pub enum PutError {
+    /// No wallet available for write operations
+    #[error("Write operations require a wallet. Use upgrade_to_read_write to add a wallet.")]
+    NoWallet,
+    /// Network-related error
+    #[error("Network error: {0}")]
+    Network(#[from] NetworkError),


There was a discussion about putting errors were they happen before, instead of all in a single file were they become un-linked to their code.
What's your take on this?

grumbach · 2024-12-30T20:15:03Z

autonomi/src/client/mod.rs

 #[derive(Clone)]
 pub struct Client {
    pub(crate) network: Network,
    pub(crate) client_event_sender: Arc<Option<mpsc::Sender<ClientEvent>>>,
    pub(crate) evm_network: EvmNetwork,
+    pub(crate) mode: ClientMode,


What's the reason behind mode here?

The mode is for local network mode (mens) or not.

grumbach · 2024-12-30T20:20:25Z

autonomi/src/client/pointer.rs

        match record {
            Some(record) => {
                let (_, pointer): (Vec<u8>, Pointer) = rmp_serde::from_slice(&record.value)
                    .map_err(|_| PointerError::Serialization)?;
                Ok(pointer)
            }
-            None => Err(PointerError::Corrupt),
+            None => Err(PointerError::FailedVerification),


Does FailedVerification cover all possible errors behind get_local_record?
I think having at least a String with some information on the actual underlying error would be extremely valuable

grumbach · 2024-12-30T20:42:09Z

autonomi/src/client/utils.rs

+        let get_chunks = move |xor_names: &[XorName]| -> Result<Vec<Bytes>, SelfEncryptionError> {
+            let mut chunks = Vec::with_capacity(xor_names.len());
+            for xor_name in xor_names {
+                match futures::executor::block_on(client.chunk_get(*xor_name)) {


Why not just await here instead of block_on?

grumbach · 2024-12-30T20:55:48Z

autonomi/src/network/local.rs

+    pub async fn start() -> Result<Self> {
+        Self::start_internal(true).await
+    }
+
+    pub async fn start_secondary() -> Result<Self> {
+        Self::start_internal(false).await
+    }


I'm not sure what start and start_secondary involves here, could this be precised?

grumbach · 2024-12-30T21:14:36Z

autonomi/tests/register.rs

-    let client = Client::init_local().await?;
+    let client = Client::init_local(true).await?;


It's not 100% clear what the true here means, if an option on/off is needed maybe another constructor or the following pattern:

Client::init_local().with_option_AAA().await?;

grumbach

Lots of great additions! Having docs in here is a real game changer!
Couple of notes and questions but overall excellent work!

…datory - Remove encrypt-records feature flag from ant-networking and ant-node\n- Make record encryption mandatory in record_store.rs\n- Update open-metrics feature to include prometheus-client\n- Remove redundant test steps in CI workflow

…eature flag as filesystem operations are now core functionality - Remove encrypted_records feature flag as encryption is now always enabled - Update tests and workflows to remove feature flag references - Clean up related code in client files and test modules

dirvine · 2024-12-31T10:47:01Z

closing as it's too big and the local flag is leading to confusion here

dirvine added DoNotMerge and removed DoNotMerge labels Dec 24, 2024

dirvine marked this pull request as ready for review December 29, 2024 01:29

grumbach reviewed Dec 30, 2024

View reviewed changes

dirvine force-pushed the data_further_refactor branch from d82ebe6 to 45d510b Compare December 30, 2024 19:51

grumbach reviewed Dec 30, 2024

View reviewed changes

dirvine added 19 commits December 30, 2024 20:08

feat: add Python bindings for ChunkAddress and Pointer types, update …

305d3b5

…version numbers, and add tests

docs: add architecture documentation and update cargo.toml

aa8ac9c

refactor: remove WASM support - Remove WASM dependencies and crate-ty…

690727f

…pe - Remove WASM test file and documentation - Update README to remove WASM references

fix: improve local network testing - Require only one peer when in lo…

d39a8c4

…cal mode instead of CLOSE_GROUP_SIZE - Remove global address check for local testing - Fix test configuration to properly use local feature - Clean up unused imports and variables

refactor: Update local network to use find_local_ip() and QUIC

babbb7d

refactor: update NetworkBuilder usage to handle local mode and bootst…

315d763

…rap cache correctly

feat: add local test suite and CI workflow

f6e1687

- Add test-local.sh script for running local tests - Add GitHub Actions workflow for running local tests - Add linked_list tests - Update client implementation for linked list operations - Clean up unused imports and fix linter warnings

fix: update test scripts to handle port conflicts

f930ce6

- Add cleanup before starting EVM testnet - Set specific RPC port for EVM testnet

feat: improve test scripts to handle existing EVM network

f640081

- Add port checks before starting EVM testnet - Use consistent port 4343 across scripts - Only clean up antnode processes, preserve EVM network

dirvine added 16 commits December 30, 2024 20:11

docs: update navigation to include all guides and improve structure

15ae771

docs: add build from source instructions for all languages

ca49d58

docs: add comprehensive API documentation for all components

0ff4358

docs: fix broken links in documentation

e6a1b21

doc: update nav

4163113

docs: reorganize documentation structure and fix links

f1166dd

fix: moved docs

5a8e5b5

doc: api nav

8081818

docs: update self-encryption API docs with streaming operations and c…

f858d2e

…hunk store implementations

docs: remove Quick Start guide and references

5da28a6

docs: add core concepts to Quick Links section

09b81ed

docs: update main documentation index with improved installation and …

5881159

…usage instructions

doc: docstring nano -> atto

bc74e67

doc: API ref

c0b6e9a

doc: index layout

2faf271

chore: update Cargo.lock after rebase

a243be6

dirvine force-pushed the data_further_refactor branch from 45d510b to a243be6 Compare December 30, 2024 20:11

github-advanced-security bot found potential problems Dec 30, 2024

View reviewed changes

grumbach reviewed Dec 30, 2024

View reviewed changes

fix: handle local feature flag in tests and fix record store tests

79fe678

grumbach reviewed Dec 30, 2024

View reviewed changes

dirvine added 2 commits December 30, 2024 21:51

dirvine closed this Dec 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Data further refactor #2574

Data further refactor #2574

dirvine commented Dec 24, 2024

grumbach Dec 30, 2024

grumbach Dec 30, 2024

dirvine Dec 30, 2024

grumbach Dec 30, 2024

dirvine Dec 30, 2024

grumbach Dec 30, 2024

grumbach Dec 30, 2024

grumbach Dec 30, 2024

dirvine Dec 30, 2024

grumbach Dec 30, 2024

dirvine Dec 30, 2024

github-advanced-security bot left a comment

grumbach Dec 30, 2024

grumbach Dec 30, 2024

dirvine Dec 30, 2024

grumbach Dec 30, 2024

grumbach Dec 30, 2024

grumbach Dec 30, 2024

grumbach Dec 30, 2024

grumbach left a comment

dirvine commented Dec 31, 2024

		/// Converts the Nanos into bytes
		pub fn to_bytes(&self) -> Vec<u8> {

		# Autonomi JS API

		Note: the JS API is experimental and will be subject to change.

		let client = Client::init_local().await?;
		let client = Client::init_local(true).await?;

Data further refactor #2574

Data further refactor #2574

Conversation

dirvine commented Dec 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-advanced-security bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grumbach left a comment

Choose a reason for hiding this comment

dirvine commented Dec 31, 2024