[Executor] CapturedReads component, Metadata and exists support

[gelash]

Create a component that exclusively deals with capturing reads from transaction execution, and validating them

• Incorporate new functionality to distinguish between reading a full value, just metadata, or only checking for existence. For now, the callers don't take full benefit of it, but resource groups will (for metadata, and it will have its own existence check for resources in the group too - functionality also implemented in CapturedReads).
• Test the component extensively.

Besides being important for getting the performance improvements for resource groups (o.w. we would get R/W conflicts on metadata), this greatly simplifies validation and code extensibility for the future, also gives us infrastructure to avoid many R/W conflicts in the future.

CapturedReads has comprehensive unit test suite. Metadata and exists reading will be integrated into proptests in a follow up PR.

[igor-aptos]

looks quite clean - it should be very easy to integrate with aggregators as well.

some questions inline:

[igor-aptos]

why is this debug assert? (and not invariant_violation)

[gelash]

UPDATED: We aren't inside the Move-VM so we can't directly throw INVARIANT_VIOLATION. Since we are in the read, we could return a fake STORAGE_ERROR to the transaction and cause it to abort, but then the question is whether we need to replace the error somehow to more proper INVARIANT_VIOLATION before it reaches the user. @zekun000, @runtian-zhou - you would know best? Maybe Storage Error with appropriate message is sufficient?

If the caller first checks to derive from an existing read (which we do, and makes sense to do), then no inconsistency should be possible (lower contained kind yes). However, I think now that asserting is maybe not good, since if it isn't the intended way now or in the future (even because of the bug), surfacing it to trigger the assert would be highly unpredictable / dependent on luck/fate.

One other option is to log an error and fall back to sequential execution. This isn't hard to do and would be useful for other cases we had in mind.
@igor-aptos @zekun000 what do you guys think?

[igor-aptos]

In general, I find reasonable to assert invariants that are localized and guaranteed to be true. (i.e. where looking at the local code we can be convinced it will never be triggered).

But here it is not really local - as it goes all the way to Unsync and Sync implementations, etc.

For such - should be - but hard to guarantee, I would be throwing special CODE_INVARIANT_BROKEN issue, that can then trigger an alert, as well as fall back to sequential execution. I am trying to see how to do that right.

But here, even more importantly - is this even an invariant? Could it be perfectly possible that read of one kind returns to the call, then another transaction before it completes and writes output, and then read for a different kind is issued, which doesn't match up here? So bail is more appropriate (does bail here require transaction re-execution)?

One stupid question, if transaction issues two reads, is there any caching, to make sure transaction sees the same thing, or will we end up in the Inconsistent case above?

[gelash]

transaction session has its own cache, that one is fully local to it, but having multiple sessions have caused weird invariant behaviors in the past (but mainly speculative violations that re-execution was fixing).

the intended use (and should be the use after this change), is that every read will first come to CapturedReads and serve from here if possible (i.e. captured read exists with >= kind), if not do the full read logic and also capture it. When served from CapturedReads it must not re-capture, so should never capture with lower kinds.
It seems like then there is one legit case when it can be inconsistent due to normal speculation - as you mentioned, when strictly lower kind is captured, and the higher kind is not consistent with it.

I like your methodology a lot, let's see how to apply here. There seem to be two distinct behaviors:

• given that inconsistency with strictly higher kind read can be speculative, in that case we can mark speculative failure (that will guarantee re-execution) and forward storage error (that will be cleared after re-execution).
• With inconsistency at the same kind of read, or having a read of a lower kind, let me alert/log an error and fallback to sequential execution. Sounds good?

[georgemitenkov]

I called it key per @zekun000's suggestion previously😂 Fine with any personally

[gelash]

and at some places Zekun said key is too ambiguous, pretty sure we don't understand the nuance :D

[zekun000]

lol I said Self::Key is ambiguous with Self::Tag or something

[georgemitenkov]

Curious, can we even have metadata when bytes are empty?

[georgemitenkov]

I think not looking at the code

[gelash]

No, and this is an ugly test type, eventually should consolidate all test types in different tests and crates (mvhashmap, etc)

[gelash]

So for real WriteOps, deletions may contain metadata, but a read following a deletion will not observe it.

[github-actions]

✅ Forge suite realistic_env_max_load success on 60dd90f369c4d5d2080b987f60920b14280fafc7

two traffics test: inner traffic : committed: 6783 txn/s, latency: 5791 ms, (p50: 5700 ms, p90: 7100 ms, p99: 13800 ms), latency samples: 2923880
two traffics test : committed: 100 txn/s, latency: 2405 ms, (p50: 2300 ms, p90: 2800 ms, p99: 4500 ms), latency samples: 1820
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.226, avg: 0.213", "QsPosToProposal: max: 0.315, avg: 0.167", "ConsensusProposalToOrdered: max: 0.645, avg: 0.588", "ConsensusOrderedToCommit: max: 0.461, avg: 0.441", "ConsensusProposalToCommit: max: 1.105, avg: 1.030"]
Max round gap was 1 [limit 4] at version 1544809. Max no progress secs was 3.770627 [limit 10] at version 1544809.
Test Ok

• Grafana dashboard
• Humio Logs
• Test runner output
• Test run is land-blocking

[github-actions]

✅ Forge suite compat success on aptos-node-v1.6.2 ==> 60dd90f369c4d5d2080b987f60920b14280fafc7

Compatibility test results for aptos-node-v1.6.2 ==> 60dd90f369c4d5d2080b987f60920b14280fafc7 (PR)
1. Check liveness of validators at old version: aptos-node-v1.6.2
compatibility::simple-validator-upgrade::liveness-check : committed: 4600 txn/s, latency: 7095 ms, (p50: 6600 ms, p90: 10700 ms, p99: 14000 ms), latency samples: 179420
2. Upgrading first Validator to new version: 60dd90f369c4d5d2080b987f60920b14280fafc7
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1815 txn/s, latency: 16177 ms, (p50: 18900 ms, p90: 22200 ms, p99: 22400 ms), latency samples: 94380
3. Upgrading rest of first batch to new version: 60dd90f369c4d5d2080b987f60920b14280fafc7
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1564 txn/s, latency: 17741 ms, (p50: 18700 ms, p90: 22800 ms, p99: 31900 ms), latency samples: 81340
4. upgrading second batch to new version: 60dd90f369c4d5d2080b987f60920b14280fafc7
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3629 txn/s, latency: 8636 ms, (p50: 9900 ms, p90: 11700 ms, p99: 12200 ms), latency samples: 145160
5. check swarm health
Compatibility test for aptos-node-v1.6.2 ==> 60dd90f369c4d5d2080b987f60920b14280fafc7 passed
Test Ok

• Grafana dashboard
• Humio Logs
• Test runner output
• Test run is land-blocking

[github-actions]

❌ Forge suite framework_upgrade failure on aptos-node-v1.5.1 ==> 60dd90f369c4d5d2080b987f60920b14280fafc7

Compatibility test results for aptos-node-v1.5.1 ==> 60dd90f369c4d5d2080b987f60920b14280fafc7 (PR)
Upgrade the nodes to version: 60dd90f369c4d5d2080b987f60920b14280fafc7
Test Failed: Unknown error error sending request for url (http://aptos-node-3-validator.forge-framework-upgrade-pr-10170.svc:8080/v1/accounts/0000000000000000000000000000000000000000000000000000000000000001/resource/0x1::features::Features): error trying to connect: tcp connect error: Connection refused (os error 111)

Stack backtrace:
   0: <core::result::Result<T,F> as core::ops::try_trait::FromResidual<core::result::Result<core::convert::Infallible,E>>>::from_residual
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/core/src/result.rs:1961:27
      aptos_release_builder::components::ReleaseEntry::validate_upgrade
             at ./aptos-move/aptos-release-builder/src/components/mod.rs:301:41
   1: aptos_release_builder::components::ReleaseConfig::validate_upgrade
             at ./aptos-move/aptos-release-builder/src/components/mod.rs:517:13
   2: aptos_release_builder::validate::execute_release::{{closure}}
             at ./aptos-move/aptos-release-builder/src/validate.rs:436:13
      aptos_release_builder::validate::validate_config_and_generate_release::{{closure}}
             at ./aptos-move/aptos-release-builder/src/validate.rs:460:6
      aptos_release_builder::validate::validate_config::{{closure}}
             at ./aptos-move/aptos-release-builder/src/validate.rs:446:80
      tokio::runtime::park::CachedParkThread::block_on::{{closure}}
             at /usr/local/cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.29.1/src/runtime/park.rs:283:63
      tokio::runtime::coop::with_budget
             at /usr/local/cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.29.1/src/runtime/coop.rs:107:5
      tokio::runtime::coop::budget
             at /usr/local/cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.29.1/src/runtime/coop.rs:73:5
      tokio::runtime::park::CachedParkThread::block_on
             at /usr/local/cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.29.1/src/runtime/park.rs:283:31
   3: tokio::runtime::context::blocking::BlockingRegionGuard::block_on
             at /usr/local/cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.29.1/src/runtime/context/blocking.rs:66:9
      tokio::runtime::scheduler::multi_thread::MultiThread::block_on::{{closure}}
             at /usr/local/cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.29.1/src/runtime/scheduler/multi_thread/mod.rs:87:13
      tokio::runtime::context::runtime::enter_runtime
             at /usr/local/cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.29.1/src/runtime/context/runtime.rs:65:16
   4: tokio::runtime::scheduler::multi_thread::MultiThread::block_on
             at /usr/local/cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.29.1/src/runtime/scheduler/multi_thread/mod.rs:86:9
      tokio::runtime::runtime::Runtime::block_on
             at /usr/local/cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.29.1/src/runtime/runtime.rs:313:50
   5: <aptos_testcases::framework_upgrade::FrameworkUpgrade as aptos_forge::interface::network::NetworkTest>::run
             at ./testsuite/testcases/src/framework_upgrade.rs:97:9
   6: aptos_forge::runner::Forge<F>::run::{{closure}}
             at ./testsuite/forge/src/runner.rs:598:42
      aptos_forge::runner::run_test
             at ./testsuite/forge/src/runner.rs:666:11
      aptos_forge::runner::Forge<F>::run
             at ./testsuite/forge/src/runner.rs:598:30
   7: forge::run_forge
             at ./testsuite/forge-cli/src/main.rs:410:11
      forge::main
             at ./testsuite/forge-cli/src/main.rs:336:21
   8: core::ops::function::FnOnce::call_once
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/core/src/ops/function.rs:250:5
      std::sys_common::backtrace::__rust_begin_short_backtrace
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/std/src/sys_common/backtrace.rs:135:18
   9: std::rt::lang_start::{{closure}}
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/std/src/rt.rs:166:18
  10: core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &F>::call_once
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/core/src/ops/function.rs:284:13
      std::panicking::try::do_call
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/std/src/panicking.rs:500:40
      std::panicking::try
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/std/src/panicking.rs:464:19
      std::panic::catch_unwind
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/std/src/panic.rs:142:14
      std::rt::lang_start_internal::{{closure}}
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/std/src/rt.rs:148:48
      std::panicking::try::do_call
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/std/src/panicking.rs:500:40
      std::panicking::try
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/std/src/panicking.rs:464:19
      std::panic::catch_unwind
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/std/src/panic.rs:142:14
      std::rt::lang_start_internal
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/std/src/rt.rs:148:20
  11: main
  12: __libc_start_main
  13: _start
Trailing Log Lines:
      std::panicking::try
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/std/src/panicking.rs:464:19
      std::panic::catch_unwind
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/std/src/panic.rs:142:14
      std::rt::lang_start_internal
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/std/src/rt.rs:148:20
  11: main
  12: __libc_start_main
  13: _start


Swarm logs can be found here: See fgi output for more information.
{"level":"INFO","source":{"package":"aptos_forge","file":"testsuite/forge/src/backend/k8s/cluster_helper.rs:292"},"thread_name":"main","hostname":"forge-framework-upgrade-pr-10170-1695754089-aptos-node-v1-5-1","timestamp":"2023-09-26T19:15:08.151195Z","message":"Deleting namespace forge-framework-upgrade-pr-10170: Some(NamespaceStatus { conditions: Some([NamespaceCondition { last_transition_time: Some(Time(2023-09-26T19:15:07Z)), message: Some(\"All resources successfully discovered\"), reason: Some(\"ResourcesDiscovered\"), status: \"False\", type_: \"NamespaceDeletionDiscoveryFailure\" }, NamespaceCondition { last_transition_time: Some(Time(2023-09-26T19:15:07Z)), message: Some(\"All legacy kube types successfully parsed\"), reason: Some(\"ParsedGroupVersions\"), status: \"False\", type_: \"NamespaceDeletionGroupVersionParsingFailure\" }, NamespaceCondition { last_transition_time: Some(Time(2023-09-26T19:15:07Z)), message: Some(\"All content successfully deleted, may be waiting on finalization\"), reason: Some(\"ContentDeleted\"), status: \"False\", type_: \"NamespaceDeletionContentFailure\" }, NamespaceCondition { last_transition_time: Some(Time(2023-09-26T19:15:07Z)), message: Some(\"Some resources are remaining: persistentvolumeclaims. has 4 resource instances\"), reason: Some(\"SomeResourcesRemain\"), status: \"True\", type_: \"NamespaceContentRemaining\" }, NamespaceCondition { last_transition_time: Some(Time(2023-09-26T19:15:07Z)), message: Some(\"Some content in the namespace has finalizers remaining: kubernetes.io/pvc-protection in 4 resource instances\"), reason: Some(\"SomeFinalizersRemain\"), status: \"True\", type_: \"NamespaceFinalizersRemaining\" }]), phase: Some(\"Terminating\") })"}
{"level":"INFO","source":{"package":"aptos_forge","file":"testsuite/forge/src/backend/k8s/cluster_helper.rs:400"},"thread_name":"main","hostname":"forge-framework-upgrade-pr-10170-1695754089-aptos-node-v1-5-1","timestamp":"2023-09-26T19:15:08.151246Z","message":"aptos-node resources for Forge removed in namespace: forge-framework-upgrade-pr-10170"}

failures:
    framework_upgrade::framework-upgrade

test result: FAILED. 0 passed; 1 failed; 0 filtered out

Failed to run tests:
Tests Failed
Error: Tests Failed

Stack backtrace:
   0: aptos_forge::runner::Forge<F>::run
             at ./testsuite/forge/src/runner.rs:618:13
   1: forge::run_forge
             at ./testsuite/forge-cli/src/main.rs:410:11
      forge::main
             at ./testsuite/forge-cli/src/main.rs:336:21
   2: core::ops::function::FnOnce::call_once
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/core/src/ops/function.rs:250:5
      std::sys_common::backtrace::__rust_begin_short_backtrace
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/std/src/sys_common/backtrace.rs:135:18
   3: std::rt::lang_start::{{closure}}
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/std/src/rt.rs:166:18
   4: core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &F>::call_once
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/core/src/ops/function.rs:284:13
      std::panicking::try::do_call
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/std/src/panicking.rs:500:40
      std::panicking::try
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/std/src/panicking.rs:464:19
      std::panic::catch_unwind
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/std/src/panic.rs:142:14
      std::rt::lang_start_internal::{{closure}}
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/std/src/rt.rs:148:48
      std::panicking::try::do_call
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/std/src/panicking.rs:500:40
      std::panicking::try
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/std/src/panicking.rs:464:19
      std::panic::catch_unwind
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/std/src/panic.rs:142:14
      std::rt::lang_start_internal
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/std/src/rt.rs:148:20
   5: main
   6: __libc_start_main
   7: _start
Debugging output:

• Grafana dashboard
• Humio Logs
• Test runner output
• Test run is land-blocking