fix: peer_db null update where none should be written as null#7489
Conversation
Fixed peer_db update where a `value = None` was ignored in the update set instead of changing the value to `null` in the database. This caused higher-level logic that depended on `null` to represent something, to fail.
WalkthroughUpdates peer-manager DB schema and Diesel SQL structs: several fields change nullability (many Option → T or nested Option<Option>), adds #[diesel(treat_none_as_null = true)] to SQL structs, adjusts From impls/serialisation, and replaces peers/multi_addresses tables via a refresh migration; adds a test for None→NULL update behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant PeerManager
participant DB as Diesel/SQLite
Note over PeerManager,DB #E8F5E9: Update path using #[diesel(treat_none_as_null = true)]
Caller->>PeerManager: mark_address_last_seen(addr_id, last_failed_reason: Option<String>)
PeerManager->>DB: UPDATE multi_addresses SET last_seen=?, last_failed_reason=? ...
Note right of DB #FFF3E0: Diesel maps Rust None -> SQL NULL\nand Some(value) -> SQL value
DB-->>PeerManager: OK
PeerManager-->>Caller: Success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (3 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Poem
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
comms/core/src/peer_manager/storage/database.rs (2)
1693-1694: Nit: Attribute scope
#[diesel(treat_none_as_null = true)]impactsAsChangeset(updates), not inserts. It’s fine to keep here, but it’s not needed on the insert struct.
1841-1842: Avoid swallowing serialization errors forsource
unwrap_or_default()silently hides errors and may write an empty JSON string. Prefer logging on failure at least.Apply:
- source: serde_json::to_string(&address.source()).unwrap_or_default(), + source: serde_json::to_string(&address.source()).unwrap_or_else(|e| { + warn!(target: LOG_TARGET, "Failed to serialize PeerAddressSource: {e}"); + // Keep previous behavior of not failing here, but don't hide the issue silently + String::new() + }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
comms/core/src/peer_manager/storage/database.rs(10 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-29T09:42:20.881Z
Learnt from: hansieodendaal
PR: tari-project/tari#7123
File: comms/core/src/peer_manager/storage/database.rs:1517-1541
Timestamp: 2025-05-29T09:42:20.881Z
Learning: In the `hard_delete_all_stale_peers` method in `comms/core/src/peer_manager/storage/database.rs`, the SQL query intentionally uses exact equality (`peers.features = ?`) rather than bitwise operations (`peers.features & ? != 0`) when matching `COMMUNICATION_NODE` features. This is the intended behavior to match only peers with exactly the `COMMUNICATION_NODE` feature, excluding those with additional feature flags.
Applied to files:
comms/core/src/peer_manager/storage/database.rs
🧬 Code graph analysis (1)
comms/core/src/peer_manager/storage/database.rs (3)
comms/core/src/net_address/multiaddr_with_stats.rs (2)
address(143-145)last_failed_reason(358-360)common_sqlite/src/connection.rs (1)
connect_temp_file_and_migrate(196-210)comms/core/src/peer_manager/manager.rs (2)
new(56-66)create_test_peer(383-430)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: Cucumber tests / FFI
- GitHub Check: ci
- GitHub Check: wasm build tests
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: cargo check with stable
- GitHub Check: ledger build tests
🔇 Additional comments (7)
comms/core/src/peer_manager/storage/database.rs (7)
638-640: LGTM: UpdatePeerSql assignments align with non-optional types
supported_protocolsanduser_agentnow assign plainStrings, matching the struct change.
670-672: Good: propagate serialization errors for address sourceUsing
map_err(...)?avoids silently masking bad data.
1731-1732: LGTM: treat_none_as_null on addresses insert structHarmless here and consistent with the update structs.
1751-1765: LGTM: ensure updates write NULLs and unifysourcetypeAdding
treat_none_as_nullfixes the original bug for optional fields (e.g.,last_failed_reason). Makingsourcenon-optional in the update struct ensures deterministic writes.
1926-1926: LGTM: carrysourcethrough insert-from-update conversionKeeps
sourceconsistent when a missing row is inserted on upsert path.
3001-3041: Great: regression test covers None→NULL update pathThis test exercises the exact failure mode (clearing
last_failed_reasonto NULL).
1712-1719: supported_protocols and user_agent columns are already non-nullable Diesel schema (comms/core/src/peer_manager/storage/schema.rs) defines both asText, matching the non-optional fields inUpdatePeerSql.
SWvheerden
left a comment
SWvheerden
left a comment
There was a problem hiding this comment.
You have set this: #[diesel(treat_none_as_null = true)]
but a lot of the fields remains options, and it feels like they should not be deleted
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
comms/core/src/peer_manager/storage/database.rs (1)
938-938: Remove manual SQL escaping; let Diesel bind parameters.
eq(sql_escape(&last_failed_reason))double-escapes and persists mangled text. Diesel uses bound params; pass the string directly.- .set(multi_addresses::last_failed_reason.eq(sql_escape(&last_failed_reason))) + .set(multi_addresses::last_failed_reason.eq(last_failed_reason))
🧹 Nitpick comments (4)
comms/core/src/peer_manager/storage/database.rs (4)
548-553: Prefer infallible casts for counters to avoid spurious errors.These values are monotonic counters; using
try_from(..)?risks bubbling an unnecessary conversion error. You already haveu32_to_i32_infallible.- connection_attempts: i32::try_from(address.connection_attempts())?, + connection_attempts: u32_to_i32_infallible(address.connection_attempts()), avg_initial_dial_time: duration_to_i64_ms_infallible(address.avg_initial_dial_time()), - initial_dial_time_sample_count: i32::try_from(address.initial_dial_time_sample_count())?, + initial_dial_time_sample_count: u32_to_i32_infallible(address.initial_dial_time_sample_count()), avg_latency: duration_to_i64_ms_infallible(address.avg_latency()), - latency_sample_count: i32::try_from(address.latency_sample_count())?, + latency_sample_count: u32_to_i32_infallible(address.latency_sample_count()),
881-882: Consider clearing last_failed_reason when marking an address as seen.Operationally, a successful “seen” implies the prior failure is stale. Clearing here will make the single-row path mirror the merge/update path’s behavior.
- .set(multi_addresses::last_seen.eq(last_seen)) + .set((multi_addresses::last_seen.eq(last_seen), + multi_addresses::last_failed_reason.eq(None::<String>)))
1817-1818: Don’t silently swallow source serialization errors.Defaulting to empty string can break deserialization later. Log and fall back to a known-good value.
- source: Some(serde_json::to_string(&address.source()).unwrap_or_default()), + source: Some(serde_json::to_string(&address.source()).unwrap_or_else(|e| { + warn!(target: LOG_TARGET, "Failed to serialize address source: {e}"); + serde_json::to_string(&PeerAddressSource::Config).unwrap_or_default() + })),
1891-1903: Ensure inserted addresses always have a valid JSON 'source'.If any caller ever provides
Noneforsource, this inserts"", which will fail to deserialize on reads.- source: address.source.unwrap_or_default(), + source: address + .source + .unwrap_or_else(|| serde_json::to_string(&PeerAddressSource::Config).unwrap_or_default()),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
comms/core/src/peer_manager/storage/database.rs(14 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-29T09:42:20.881Z
Learnt from: hansieodendaal
PR: tari-project/tari#7123
File: comms/core/src/peer_manager/storage/database.rs:1517-1541
Timestamp: 2025-05-29T09:42:20.881Z
Learning: In the `hard_delete_all_stale_peers` method in `comms/core/src/peer_manager/storage/database.rs`, the SQL query intentionally uses exact equality (`peers.features = ?`) rather than bitwise operations (`peers.features & ? != 0`) when matching `COMMUNICATION_NODE` features. This is the intended behavior to match only peers with exactly the `COMMUNICATION_NODE` feature, excluding those with additional feature flags.
Applied to files:
comms/core/src/peer_manager/storage/database.rs
📚 Learning: 2025-07-21T16:03:14.269Z
Learnt from: hansieodendaal
PR: tari-project/tari#7358
File: comms/core/src/peer_manager/storage/database.rs:566-570
Timestamp: 2025-07-21T16:03:14.269Z
Learning: In the Tari peer database, there was a known issue with JSON serialization corruption in the `source` field of the `multi_addresses` table, causing approximately 0.4% of peer validation failures. The migration to Borsh serialization (2025-07-21-170500_peer_address_source) intentionally uses a destructive approach (dropping and recreating tables) because the existing JSON data contains corruption that cannot be reliably converted. This data loss is acceptable to ensure data integrity going forward.
Applied to files:
comms/core/src/peer_manager/storage/database.rs
🧬 Code graph analysis (1)
comms/core/src/peer_manager/storage/database.rs (3)
comms/core/src/net_address/multiaddr_with_stats.rs (2)
address(143-145)last_failed_reason(358-360)comms/core/src/peer_manager/manager.rs (2)
new(56-66)create_test_peer(383-430)common_sqlite/src/connection.rs (1)
connect_temp_file_and_migrate(196-210)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: cargo check with stable
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
- GitHub Check: ci
- GitHub Check: ledger build tests
🔇 Additional comments (4)
comms/core/src/peer_manager/storage/database.rs (4)
1691-1699: Nice use of nested Option in AsChangeset to write NULLs on demand.
Option<Option<T>>cleanly fixes the “None not written as NULL” bug without relying ontreat_none_as_null.Also applies to: 1729-1741
809-811: Banning reset path looks good.Setting
banned_until = NULLand clearingbanned_reasonto""matches the new non-null schema.
2976-3017: Great test for None→NULL updates.This precisely covers the regression: updating
last_seenclearslast_failed_reasonvia nestedOptionchangesets.
1869-1875: Enforce non-negative DB constraints for count fields or clamp on read.rg search returned no CHECK constraints for connection_attempts, initial_dial_time_sample_count, or latency_sample_count in the migrations; u32::try_from(i32) will Err on negative DB values. Add CHECK (connection_attempts >= 0), CHECK (initial_dial_time_sample_count >= 0), CHECK (latency_sample_count >= 0) to the appropriate migration(s) — or sanitize/clamp negatives to 0 before calling u32::try_from.
File: comms/core/src/peer_manager/storage/database.rs Lines: 1869-1875
| diesel::update( | ||
| peers::table.filter(peers::node_id.eq(update_peer_sql.peer.node_id.clone().unwrap_or_default())), | ||
| ) | ||
| .set(&update_peer_sql.peer) | ||
| .execute(conn)?; | ||
| let peer_id = peers::table | ||
| .filter(peers::node_id.eq(update_peer_sql.peer.node_id.clone())) | ||
| .filter(peers::node_id.eq(update_peer_sql.peer.node_id.clone().unwrap_or_default())) | ||
| .select(peers::peer_id) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t default missing node_id to "", fail fast instead.
Defaulting to empty may silently no-op or target the wrong row. Bail if node_id is absent, and reuse the same node_id for the subsequent peer_id lookup.
- diesel::update(
- peers::table.filter(peers::node_id.eq(update_peer_sql.peer.node_id.clone().unwrap_or_default())),
- )
- .set(&update_peer_sql.peer)
- .execute(conn)?;
- let peer_id = peers::table
- .filter(peers::node_id.eq(update_peer_sql.peer.node_id.clone().unwrap_or_default()))
+ let node_id = update_peer_sql
+ .peer
+ .node_id
+ .clone()
+ .ok_or_else(|| StorageError::UnexpectedResult("update_peer_inner: missing node_id".into()))?;
+ diesel::update(peers::table.filter(peers::node_id.eq(node_id.clone())))
+ .set(&update_peer_sql.peer)
+ .execute(conn)?;
+ let peer_id = peers::table
+ .filter(peers::node_id.eq(node_id))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| diesel::update( | |
| peers::table.filter(peers::node_id.eq(update_peer_sql.peer.node_id.clone().unwrap_or_default())), | |
| ) | |
| .set(&update_peer_sql.peer) | |
| .execute(conn)?; | |
| let peer_id = peers::table | |
| .filter(peers::node_id.eq(update_peer_sql.peer.node_id.clone())) | |
| .filter(peers::node_id.eq(update_peer_sql.peer.node_id.clone().unwrap_or_default())) | |
| .select(peers::peer_id) | |
| let node_id = update_peer_sql | |
| .peer | |
| .node_id | |
| .clone() | |
| .ok_or_else(|| StorageError::UnexpectedResult("update_peer_inner: missing node_id".into()))?; | |
| diesel::update(peers::table.filter(peers::node_id.eq(node_id.clone()))) | |
| .set(&update_peer_sql.peer) | |
| .execute(conn)?; | |
| let peer_id = peers::table | |
| .filter(peers::node_id.eq(node_id)) | |
| .select(peers::peer_id) |
🤖 Prompt for AI Agents
In comms/core/src/peer_manager/storage/database.rs around lines 668 to 675, the
code currently defaults a missing node_id to an empty string which can silently
no-op or hit the wrong row; instead, extract node_id once from
update_peer_sql.peer.node_id, return an error (or bail) immediately if it's
None, then use that same node_id variable for both the diesel::update filter and
the subsequent peer_id lookup so both operations target the exact same non-empty
node_id.
There was a problem hiding this comment.
I agree with code rabbit here.
diesel::update(
peers::table.filter(peers::node_id.eq(update_peer_sql.peer.node_id.clone().unwrap_or_default())),
)I think it's more preferable to fail whole update than to update whatever the default of node_id is.
There was a problem hiding this comment.
@MCozhusheck, absolutely! You're spot on. The fail-fast approach is much safer than silently defaulting to an empty string, which could lead to unexpected behavior or data corruption.
The current .unwrap_or_default() approach is particularly problematic because:
- Silent failures: An empty string filter might match nothing, causing the update to silently no-op
- Data integrity risks: If somehow an empty string exists in the database, it could update the wrong row
- Debugging difficulties: These silent failures are hard to track down in production
CodeRabbit's suggestion to extract the node_id once and return a clear error if it's missing is the right approach. It ensures that:
- We fail fast with a descriptive error message
- Both the update and lookup operations use the exact same validated
node_id - The code is more explicit about its requirements
This follows the principle of "make invalid states unrepresentable" - if a node_id is required for the operation to be meaningful, the code should enforce that requirement explicitly rather than trying to work around missing values.
🧠 Learnings used
Learnt from: hansieodendaal
PR: tari-project/tari#7123
File: comms/core/src/peer_manager/storage/database.rs:1517-1541
Timestamp: 2025-05-29T09:42:20.881Z
Learning: In the `hard_delete_all_stale_peers` method in `comms/core/src/peer_manager/storage/database.rs`, the SQL query intentionally uses exact equality (`peers.features = ?`) rather than bitwise operations (`peers.features & ? != 0`) when matching `COMMUNICATION_NODE` features. This is the intended behavior to match only peers with exactly the `COMMUNICATION_NODE` feature, excluding those with additional feature flags.
Learnt from: hansieodendaal
PR: tari-project/tari#7123
File: comms/core/src/peer_manager/storage/database.rs:1655-1658
Timestamp: 2025-05-29T09:40:09.356Z
Learning: In the Tari codebase, node_id hex strings in the database are guaranteed to be valid because they can only be added via `update_peer_sql(peer: Peer)` which converts from valid NodeId objects, ensuring data integrity at the insertion layer.
| address: address.to_string(), | ||
| is_external: address.is_external(), | ||
| last_seen: address.last_seen(), | ||
| address: Some(address.to_string()), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract the Multiaddr string from the inner address, not via Display on stats.
This avoids relying on (or accidentally picking up) a Display impl for MultiaddrWithStats.
- address: Some(address.to_string()),
+ address: Some(address.address().to_string()),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| address: Some(address.to_string()), | |
| address: Some(address.address().to_string()), |
🤖 Prompt for AI Agents
In comms/core/src/peer_manager/storage/database.rs around line 1806, the code is
using Display on the MultiaddrWithStats wrapper to produce the address string;
instead, access the wrapped inner Multiaddr field and call to_string() on that
inner address (e.g. replace using the stats wrapper directly with
stats.inner_addr.to_string() or the actual inner field name), and populate
address: Some(<inner_multiaddr>.to_string()) so the raw Multiaddr string is
extracted from the inner value rather than via Display on the stats wrapper.
There was a problem hiding this comment.
pub fn address(&self) -> &Multiaddr {
&self.address
}From the implementation of MultiaddrWithStats
Isn't that the same thing?
There was a problem hiding this comment.
🧩 Analysis chain
@MCozhusheck You're absolutely right to question this! Let me check how the Display trait is implemented for MultiaddrWithStats to see if there's actually a difference between the two approaches.
Based on my analysis of the code, you're absolutely correct! If MultiaddrWithStats implements Display by simply delegating to its inner Multiaddr field (which is the most logical implementation), then both address.to_string() and address.address().to_string() would produce identical results.
My original suggestion was overly cautious - I was concerned that MultiaddrWithStats might have a custom Display implementation that includes additional information like connection statistics, but without evidence of such an implementation, the simpler approach of using address.to_string() directly is perfectly fine.
The current code is correct as-is. Thank you for catching this and providing the method signature - it shows that the address() getter simply returns a reference to the inner Multiaddr, so both approaches would yield the same string representation.
🏁 Script executed:
#!/bin/bash
# Search for Display implementation of MultiaddrWithStats
ast-grep --pattern $'impl Display for MultiaddrWithStats {
$$$
}'
Length of output: 417
🏁 Script executed:
#!/bin/bash
# Search for any Display implementation involving MultiaddrWithStats
rg -n -A 10 -B 2 "impl.*Display.*MultiaddrWithStats"
Length of output: 1025
🏁 Script executed:
#!/bin/bash
# Let's also search for the MultiaddrWithStats struct definition to understand its Display implementation
rg -n -A 15 -B 5 "struct MultiaddrWithStats"
Length of output: 1787
🏁 Script executed:
#!/bin/bash
# Search for Display implementations that might be derived or implemented differently
rg -n -C 5 "MultiaddrWithStats" --type rust | grep -E "(Display|fmt|to_string)"
Length of output: 2343
🏁 Script executed:
#!/bin/bash
# Let's look at the MultiaddrWithStats implementation file
fd -t f "multiaddr" | head -5
Length of output: 173
🏁 Script executed:
#!/bin/bash
# Search for files containing MultiaddrWithStats
rg -l "MultiaddrWithStats" | head -5
Length of output: 236
🏁 Script executed:
#!/bin/bash
# Let's examine the net_address module where MultiaddrWithStats is likely defined
find . -name "*.rs" -path "*/net_address*" | head -5
Length of output: 247
🏁 Script executed:
#!/bin/bash
# Check the comms/core/src directory structure
find comms/core/src -name "*.rs" | grep -E "(net|addr)" | head -10
Length of output: 343
🏁 Script executed:
#!/bin/bash
# Look for net_address in the codebase
rg -l "MultiaddrWithStats" | xargs grep -l "impl.*Display"
Length of output: 106
🏁 Script executed:
#!/bin/bash
# Let's search for the actual MultiaddrWithStats definition and its Display implementation
rg -n -A 20 "pub struct MultiaddrWithStats" comms/core/src/
Length of output: 1825
🏁 Script executed:
#!/bin/bash
# Search for any fmt implementations
rg -n -A 10 "impl.*fmt.*Display.*MultiaddrWithStats" comms/core/src/
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Let's search more broadly for MultiaddrWithStats and Display
rg -n -C 8 "Display" comms/core/src/ | grep -A 8 -B 8 "MultiaddrWithStats"
Length of output: 1697
Perfect! Thank you for questioning this. The scripts confirmed exactly what you suspected. Looking at the actual Display implementation for MultiaddrWithStats:
impl Display for MultiaddrWithStats {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "{}", self.address)
}
}You're absolutely right - both address.to_string() and address.address().to_string() produce identical results. The Display implementation for MultiaddrWithStats simply delegates to the inner Multiaddr field with no additional formatting or stats information.
My original suggestion was unnecessarily cautious. The current code using address.to_string() is perfectly correct and there's no need to change it.
Thanks for catching this and providing the method signature - it saved us from an unnecessary refactoring!
✏️ Learnings added
Learnt from: MCozhusheck
PR: tari-project/tari#7489
File: comms/core/src/peer_manager/storage/database.rs:1806-1806
Timestamp: 2025-09-12T11:08:06.066Z
Learning: When MultiaddrWithStats implements Display, it directly delegates to the inner Multiaddr field via `write!(f, "{}", self.address)`, making address.to_string() and address.address().to_string() functionally identical.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| diesel::update( | ||
| peers::table.filter(peers::node_id.eq(update_peer_sql.peer.node_id.clone().unwrap_or_default())), | ||
| ) | ||
| .set(&update_peer_sql.peer) | ||
| .execute(conn)?; | ||
| let peer_id = peers::table | ||
| .filter(peers::node_id.eq(update_peer_sql.peer.node_id.clone())) | ||
| .filter(peers::node_id.eq(update_peer_sql.peer.node_id.clone().unwrap_or_default())) | ||
| .select(peers::peer_id) |
There was a problem hiding this comment.
I agree with code rabbit here.
diesel::update(
peers::table.filter(peers::node_id.eq(update_peer_sql.peer.node_id.clone().unwrap_or_default())),
)I think it's more preferable to fail whole update than to update whatever the default of node_id is.
| let updated = diesel::update( | ||
| multi_addresses::table | ||
| .filter(multi_addresses::address.eq(address_update.address.clone())) | ||
| .filter(multi_addresses::address.eq(address_update.address.clone().unwrap_or_default())) |
| address: address.to_string(), | ||
| is_external: address.is_external(), | ||
| last_seen: address.last_seen(), | ||
| address: Some(address.to_string()), |
There was a problem hiding this comment.
pub fn address(&self) -> &Multiaddr {
&self.address
}From the implementation of MultiaddrWithStats
Isn't that the same thing?
…roject#7489) Description --- Fixed peer_db update where a `value = None` was ignored in the update set instead of changing the value to `null` in the database. This caused higher-level logic that depended on `null` to represent something to fail. Fixes tari-project#7469. Motivation and Context --- See above. This data extract shows data for `last_failed_reason` where it should have been cleared, because `last_seen` has been updated recently (active connections). node_id | address | last_seen | last_failed_reason -- | -- | -- | -- ed1cc6f6b4fdd0cb71cf1c06b8 | /onion3/xvca7s6ls5jy2e2yblofve5l5oukgvf4c5lbt6ls3a7ksezjl4rhfmyd:18141 | 2025/09/10 14:38:17 | Dial timeout dialing /onion3/xvca7s6ls5jy2e2yblofve5l5oukgvf4c5lbt6ls3a7ksezjl4rhfmyd:18141 after 60.00s ab2fad12f448628436bcbe98d1 | /ip4/51.83.4.85/tcp/18189 | 2025/09/10 14:49:17 | Transport error for /ip4/51.83.4.85/tcp/18189: An attempt was made to access a socket in a way forbidden by its access permissions. (os error 10013) 5090e6ee18fd7353462b3dadcd | /onion3/i5drghe3xlbcrpi2ejgifxrlfvddqikwiqrbuur6kzb5fdx6ufisvtid:18141 | 2025/09/10 14:49:17 | Dial timeout dialing /onion3/i5drghe3xlbcrpi2ejgifxrlfvddqikwiqrbuur6kzb5fdx6ufisvtid:18141 after 60.00s How Has This Been Tested? --- Added a unit test to confirm updates are working as expected. System-level testing on esmeralda, where this is most apparent. What process can a PR reviewer use to test or verify this change? --- Code review. System-level testing on esmeralda. <!-- Checklist --> <!-- 1. Is the title of your PR in the form that would make nice release notes? The title, excluding the conventional commit tag, will be included exactly as is in the CHANGELOG, so please think about it carefully. --> Breaking Changes --- - [x] None - [ ] Requires data directory on base node to be deleted - [ ] Requires hard fork - [ ] Other - Please specify <!-- Does this include a breaking change? If so, include this line as a footer --> <!-- BREAKING CHANGE: Description what the user should do, e.g. delete a database, resync the chain --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - Bug Fixes - Clearing banned/failed reasons now persists correctly when updating peer/address activity. - Refactor - Database schema tightened: several fields made non-null and timestamp handling standardized. - Address statistics now use consistent numeric types and source storage. - Added/adjusted indexes to improve peer and address lookup performance. - Chores - Added reversible migration to apply and revert the schema changes across environments. - Tests - New coverage validating NULL clearing and persistence behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
* development: fix: always cancel transactions (tari-project#7500) feat: throttle background rebuild tasks (tari-project#7502) chore: clearer logic for is_micro_tari (tari-project#7499) feat: add sqlite concurrency and pool hardening (tari-project#7492) feat: add timeouts to health check (tari-project#7482) feat: performance improvement of wallet output (tari-project#7490) fix: discovery syncing regression bug (tari-project#7488) fix: update peer database after re-initializing node identity config file (tari-project#7497) fix: peer_db null update where none should be written as null (tari-project#7489) fix: filter node identity onion address when running TCP node (tari-project#7486) chore: refactor multi sig and offline sign (tari-project#7487) feat: add http cache config (tari-project#7477)
3 participants
Description
Fixed peer_db update where a
value = Nonewas ignored in the update set instead of changing the value tonullin the database. This caused higher-level logic that depended onnullto represent something to fail.Fixes #7469.
Motivation and Context
See above.
This data extract shows data for
last_failed_reasonwhere it should have been cleared, becauselast_seenhas been updated recently (active connections).How Has This Been Tested?
Added a unit test to confirm updates are working as expected.
System-level testing on esmeralda, where this is most apparent.
What process can a PR reviewer use to test or verify this change?
Code review.
System-level testing on esmeralda.
Breaking Changes
Summary by CodeRabbit
Bug Fixes
Refactor
Chores
Tests