Lock-free followup
- Dominant language
- Rust
- Stars
- 104
- Forks
- 138
- Avg merge
- 1d 13h
- Merged PRs (30d)
- 56
Description
This is fine but this gets logged only once the view guard is dropped. IIUC, there could be a single 1-hour long request and a total of 3 alive views and we would never know, all while RocksDB garbage accumulates. Consider checking in some other places. For example, once per applied block when doing `prune_tip`
https://github.com/0xMiden/node/pull/2345#discussion_r3732172154
---
The following was solved by https://github.com/0xMiden/node/pull/2438.
I think this could now potentially be complicated for pinned queries. The reasoning is that the `prune_history` prunes data but does _not_ ensure there is a valid row below that chain tip unless `is_latest = true`.
In the client, we solved a similar issue by transforming `is_latest` into `was_replaced_at` (`at` here being an account nonce, but it could be a block number in the case of the node). This lets you make a better decision when pruning and comes with pretty much no overhead. The idea is that you can now know the associated validity block interval for thte row, so when pruning you can check what the latest was by block N (N=`prune_tip`) and keep that one.
To be clear, this is the mechanism (asked Claude to come up with a clear example, let me know if it's good enough):
1. A read request arrives — `get_account`, say. It pins the generation-900 view. Because it's pinned, `prune_tip = 900`.
2. Block 901 commits. It updates `(A, K)`. Inside the one `apply_block` transaction:
* the upsert flips the old row: `(A, K, 100)` goes from `is_latest = true` to `false`;
* the new row `(A, K, 901, is_latest = true)` is inserted;
* `prune_history(900)` runs: `DELETE WHERE block_num < 850 AND is_latest = false`. Row `(A, K, 100)` matches — flipped one statement ago, `100 < 850` — and is deleted.
3. The still-pinned view at 900 reconstructs A's vault: for key `K`, `MAX(block_num ≤ 900)` finds nothing — the 100-row is gone, and the 901-row is excluded by the scope. Asset `K` silently vanishes from the vault as of block 900.
I believe this was a problem before as well BTW, but not sure.
https://github.com/0xMiden/node/pull/2345#discussion_r3732246652
---
LGTM! Left a couple of comments. One of them I think could be a bug (https://github.com/0xMiden/node/pull/2345/changes#r3732246652), but may be fixed separately.
I think this PR sets up streaming endpoints even better, but we'll have to check that this does not result in a lot of pinned snapshots (could re-pin in between pages or something like that).
The other thing I'm not entirely sure about is whether there should be more observability. I'd err on the verbose side and push enough data to know whether things are losing stability sooner rather than later, and how these changes affect resources, etc. This is specially true with the MMR clones in mind.
_Originally posted by @igamigo in https://github.com/0xMiden/node/pull/2345#pullrequestreview-4878490286_
Contributor guide
Assessment
This issue has not been assessed yet.