Feature/immediately handle subintent nullification

[dhedey]

Summary

The main intention was to ensure subintent handling is fixed/immediate in the pending result cache and the mempool.

As part of this ticket, I worked through a few refactors:

• Encapsulated mempool related systems directly under the MempoolManager where it can (A) safely handle atomicity of operations and avoid deadlocks, and (B) enable us to eventually safely refactor and possibly merge the cache and mempool. That was the intended direction ~18 months ago, I'm not quite sure why it ended up not being finished - but I've pushed through with the merger anyway. I think it works nicely.
• Improved the scope of our locks to be smaller. In one particular case in add_if_committable_internal I expect this to have a non-trivial impact on throughput.
• Improved the pending rejection "is permanent" checks to require passing an AtState so we can't use the wrong thing by mistake; and commonise the code.

Testing

• Existing tests pass
• Fixed the test added in test: Add test for duplicate submission of subintent #1015

Might add some more unit tests tomorrow.

[github-actions]

Phylum Report Link

[github-actions]

Docker tags
docker.io/radixdlt/private-babylon-node:pr-1016
docker.io/radixdlt/private-babylon-node:35688e4d64
docker.io/radixdlt/private-babylon-node:sha-35688e4

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 39.06250% with 351 lines in your changes missing coverage. Please review.

Project coverage is 42.7%. Comparing base (befe08d) to head (851b910).
Report is 2 commits behind head on develop.

Files with missing lines	Patch %	Lines
...er/src/mempool/pending_transaction_result_cache.rs	56.5%	116 Missing ⚠️
...-rust/state-manager/src/mempool/mempool_manager.rs	24.0%	98 Missing ⚠️
...e-rust/state-manager/src/transaction/validation.rs	5.6%	67 Missing ⚠️
...rust/state-manager/src/mempool/priority_mempool.rs	18.1%	36 Missing ⚠️
core-rust/state-manager/src/mempool/mod.rs	0.0%	5 Missing ⚠️
...er/src/core_api/handlers/lts/transaction_status.rs	0.0%	4 Missing ⚠️
...erver/src/core_api/handlers/mempool_transaction.rs	0.0%	4 Missing ⚠️
...server/src/core_api/handlers/transaction_status.rs	0.0%	4 Missing ⚠️
core-rust/state-manager/src/committer.rs	60.0%	4 Missing ⚠️
core-rust/state-manager/src/jni/mempool.rs	0.0%	4 Missing ⚠️
... and 6 more

Additional details and impacted files

@@             Coverage Diff             @@
##             develop   #1016     +/-   ##
===========================================
- Coverage       42.8%   42.7%   -0.2%     
- Complexity      4621    4623      +2     
===========================================
  Files           1759    1759             
  Lines          54252   54468    +216     
  Branches        1524    1524             
===========================================
+ Hits           23266   23302     +36     
- Misses         30505   30686    +181     
+ Partials         481     480      -1     

Flag	Coverage Δ
rust	42.7% <39.0%> (-0.2%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...er/src/core_api/handlers/lts/transaction_submit.rs	0.0% <ø> (ø)
...e-api-server/src/core_api/handlers/mempool_list.rs	0.0% <ø> (ø)
core-rust/p2p/src/address_book_components.rs	22.2% <ø> (ø)
core-rust/p2p/src/migration.rs	100.0% <ø> (ø)
core-rust/p2p/src/safety_store_components.rs	5.5% <ø> (ø)
...ust/state-manager/src/jni/node_rust_environment.rs	0.0% <ø> (ø)
core-rust/state-manager/src/store/traits.rs	37.0% <ø> (ø)
core-rust/state-manager/src/types.rs	50.5% <ø> (ø)
...-server/src/core_api/handlers/transaction_parse.rs	0.0% <0.0%> (ø)
core-rust/state-manager/src/state_manager.rs	86.6% <94.1%> (-0.1%)	⬇️
... and 14 more

... and 2 files with indirect coverage changes

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[iamyulong]

Per cuttlefish update, transaction validation is stateful. Is the permanent rejection logic below still correct?

[dhedey]

Not quite - if a Cuttlefish-only transaction is evaluated against Bottlenose it will be permanently rejected at validation time, but then become acceptable in Cuttlefish.

I think this is unlikely to cause any critical issues -- and have some ideas for tweaking this in future, as discussed in the task list document -- but it's quite a hard subject to resolve satisfactorily without a lot of work. Happy to discuss later in the week.

[iamyulong]

Is mempool transaction adding single-threaded? The "sync" logic doesn't look thread-safe - there is a race condition where the reverse lookup goes out-of-sync with pending transaction records.

[dhedey]

Inside the pending_transaction_result_cache.rs, we have a &mut self so know we have locally exclusive access - the locking and co-ordination is controlled in the mempool_manager.rs parent -- but please do verify it for correctness :)

[iamyulong]

You're right. The &mut references are obtained from write() on the RW Lock, not some unsafe transmutation, which prevents unsynchronized modification to the cache data structure.