Add new command line option to make replaying transactions easier #5027

Bronek · 2024-05-23T14:18:28Z

High Level Overview of Change

Add new command line option trap_tx_hash to make replaying transactions easier

This new option can be used only if replay is also enabled. It takes a transaction hash from the ledger loaded for replay, and will cause a specific line to be hit in Transactor::operator(), right before apply().

Emit an error if:

the new option is used without replay
the supplied parameter cannot be parsed as transaction hash (i.e. uint256 in hex)
the supplied transaction hash is not found on the ledger being replayed

Context of Change

While replaying specific ledger is relatively easy, things get more difficult if we want to debug a specific transaction within the ledger. There are several options, but all of them are a hassle. Why not just a new option to rippled to hit a very specific line of code, right before a specific transaction on the replayed ledger is being applied.

This change adds new virtual function std::optional<uint256> trapTxID() const to ripple::Application class. This new function is called inside Transactor::operator() to check if:

trap transaction is set
the currently processed transaction is a match

If both conditions are met, rippled will simply log Transaction trapped followed by a hash. The user can put a breakpoint at this line to break into the debugger.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Performance (increase or change in throughput and/or latency)
Tests (you added tests for code that already exists, or your new feature included in this PR)
Documentation update
Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
Release

This new option can be used only if replay is also enabled. It takes a transaction hash from the ledger loaded for replay, and will cause a specific line to be hit in Transactor.cpp, right before the selected transaction is applied.

codecov-commenter · 2024-05-23T15:53:10Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.3%. Comparing base (e3d1bb2) to head (add69ab).

❗ Current head add69ab differs from pull request most recent head 79dff4b

Please upload reports for the commit 79dff4b to get more accurate results.

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5027     +/-   ##
=========================================
+ Coverage     71.1%   71.3%   +0.2%     
=========================================
  Files          796     796             
  Lines        66997   66883    -114     
  Branches     10979   10867    -112     
=========================================
+ Hits         47635   47685     +50     
+ Misses       19362   19198    -164

Files	Coverage Δ
src/ripple/app/main/Application.cpp	`65.9% <100.0%> (+2.9%)`	⬆️
src/ripple/app/main/Application.h	`100.0% <ø> (ø)`
src/ripple/app/main/Main.cpp	`79.6% <100.0%> (+32.6%)`	⬆️
src/ripple/app/tx/impl/Transactor.cpp	`84.6% <100.0%> (+0.1%)`	⬆️
src/ripple/core/Config.h	`77.8% <ø> (ø)`

... and 7 files with indirect coverage changes

Bronek · 2024-05-24T14:42:01Z

I will try to add add unit test for this change, since apparently Application class is usable in unit tests.

src/ripple/app/main/Application.cpp

src/ripple/app/tx/impl/Transactor.cpp

src/ripple/core/Config.h

gregtatcam · 2024-06-11T11:56:22Z

src/ripple/app/main/Application.cpp

+                JLOG(m_journal.fatal())
+                    << "Ledger " << replayLedger->info().seq
+                    << " does not contain the transaction hash " << *trapTxID;
+                return false;


Should we return false in this case or just log? The semantics of this call is to return false if the ledger fails to load.

Yes, that's intentional - you only use this functionality when replaying a transaction, and failing a little earlier (still need several minutes to load that ledger) will save your time.

gregtatcam · 2024-06-11T14:22:57Z

src/test/app/LedgerLoad_test.cpp

+
+        env.close();
+        auto const closed = env.rpc("ledger", "current", "full")[jss::result];
+        BEAST_EXPECT(closed[jss::ledger][jss::accountState].size() == 98);


What is the meaning of the tests for 97, 98, "<=", etc? Can you add some comments to clarify. Just adding the comments in one code block would be useful.

Added comment to explain - in replay mode, the ledger being replayed needs to be explicitly accepted e.g. with env.close() . Before this call, we are getting an earlier ledger as current, with one fewer state objects.

seelabs

I'm a little torn on this patch. On the one hand, this makes it easier to replay transactions without needing to set conditional breakpoints. On the other hand, knowing how to set conditional breakpoints is a skill I'd expect anyone wanting to replay transactions to have. On the other hand, this is pretty simple code, so adding it isn't too large of a cost.

If other people find this useful I think it's OK to add.

seelabs

👍

* upstream/develop: (32 commits) fixInnerObjTemplate2 amendment (XRPLF#5047) Set version to 2.3.0-b1 Ignore restructuring commits (XRPLF#4997) Recompute loops (XRPLF#4997) Rewrite includes (XRPLF#4997) Rearrange sources (XRPLF#4997) Move CMake directory (XRPLF#4997) Add bin/physical.sh (XRPLF#4997) Prepare to rearrange sources: (XRPLF#4997) Change order of checks in amm_info: (XRPLF#4924) Add the fixEnforceNFTokenTrustline amendment: (XRPLF#4946) Replaces the usage of boost::string_view with std::string_view (XRPLF#4509) docs: explain how to find a clang-format patch generated by CI (XRPLF#4521) XLS-52d: NFTokenMintOffer (XRPLF#4845) chore: remove repeat words (XRPLF#5041) Expose all amendments known by libxrpl (XRPLF#5026) fixReducedOffersV2: prevent offers from blocking order books: (XRPLF#5032) Additional unit tests for testing deletion of trust lines (XRPLF#4886) Fix conan typo: (XRPLF#5044) Add new command line option to make replaying transactions easier: (XRPLF#5027) ...

…RPLF#5027) * Add trap_tx_hash command line option This new option can be used only if replay is also enabled. It takes a transaction hash from the ledger loaded for replay, and will cause a specific line to be hit in Transactor.cpp, right before the selected transaction is applied.

Add trap_tx_hash command line option

063da61

This new option can be used only if replay is also enabled. It takes a transaction hash from the ledger loaded for replay, and will cause a specific line to be hit in Transactor.cpp, right before the selected transaction is applied.

Bronek force-pushed the feature/add_trap_tx_hash branch from 297bc96 to d87bcca Compare May 23, 2024 15:20

Simplification

f62f663

Bronek force-pushed the feature/add_trap_tx_hash branch from d87bcca to f62f663 Compare May 23, 2024 15:21

Bronek marked this pull request as ready for review May 24, 2024 14:40

Bronek requested a review from thejohnfreeman May 24, 2024 14:40

Add unit test

92ca3f8

Bronek force-pushed the feature/add_trap_tx_hash branch from 2b7b7f7 to 92ca3f8 Compare May 29, 2024 15:55

exunda reviewed May 29, 2024

View reviewed changes

src/ripple/app/main/Application.cpp Outdated Show resolved Hide resolved

src/ripple/app/main/Application.cpp Outdated Show resolved Hide resolved

src/ripple/app/tx/impl/Transactor.cpp Outdated Show resolved Hide resolved

src/ripple/core/Config.h Outdated Show resolved Hide resolved

Bronek added 3 commits May 29, 2024 19:20

Mark untestable area in Main.cpp

4b61743

Review feedback

412dad1

Enable bool shortcircuit again

edd07ea

thejohnfreeman approved these changes Jun 7, 2024

View reviewed changes

gregtatcam reviewed Jun 11, 2024

View reviewed changes

seelabs reviewed Jun 11, 2024

View reviewed changes

seelabs approved these changes Jun 11, 2024

View reviewed changes

seelabs and others added 2 commits June 11, 2024 13:27

Merge branch 'develop' into feature/add_trap_tx_hash

add69ab

Minor improvements from review

79dff4b

Bronek force-pushed the feature/add_trap_tx_hash branch from 585268c to 79dff4b Compare June 11, 2024 18:17

seelabs merged commit d576416 into XRPLF:develop Jun 11, 2024
16 of 17 checks passed

seelabs mentioned this pull request Jun 20, 2024

Proposed 2.3.0-b1 #5050

Merged

1 task

Bronek mentioned this pull request Aug 2, 2024

refactor: Factor out Transactor::trapTransaction #5087

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new command line option to make replaying transactions easier #5027

Add new command line option to make replaying transactions easier #5027

Bronek commented May 23, 2024 •

edited

Loading

codecov-commenter commented May 23, 2024 •

edited by codecov bot

Loading

Bronek commented May 24, 2024

gregtatcam Jun 11, 2024

Bronek Jun 11, 2024

gregtatcam Jun 11, 2024

Bronek Jun 11, 2024

seelabs left a comment

seelabs left a comment

Add new command line option to make replaying transactions easier #5027

Add new command line option to make replaying transactions easier #5027

Conversation

Bronek commented May 23, 2024 • edited Loading

High Level Overview of Change

Context of Change

Type of Change

codecov-commenter commented May 23, 2024 • edited by codecov bot Loading

Codecov Report

Bronek commented May 24, 2024

gregtatcam Jun 11, 2024

Choose a reason for hiding this comment

Bronek Jun 11, 2024

Choose a reason for hiding this comment

gregtatcam Jun 11, 2024

Choose a reason for hiding this comment

Bronek Jun 11, 2024

Choose a reason for hiding this comment

seelabs left a comment

Choose a reason for hiding this comment

seelabs left a comment

Choose a reason for hiding this comment

Bronek commented May 23, 2024 •

edited

Loading

codecov-commenter commented May 23, 2024 •

edited by codecov bot

Loading