From e1d97bea12c6ebc3dc5cda19985c3c28987fb1fe Mon Sep 17 00:00:00 2001 From: Bart Date: Thu, 8 Jan 2026 15:02:59 -0500 Subject: [PATCH 01/11] ci: Use updated prepare-runner in actions and worfklows (#6188) This change updates the XRPLF pre-commit workflow and prepare-runner action to their latest versions. For naming consistency the prepare-runner action changed the disable_ccache variable into enable_ccache, which matches our naming. --- .github/workflows/pre-commit.yml | 2 +- .github/workflows/reusable-build-test-config.yml | 4 ++-- .github/workflows/upload-conan-deps.yml | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/pre-commit.yml b/.github/workflows/pre-commit.yml index d0a657dd7e..41e82fb6bb 100644 --- a/.github/workflows/pre-commit.yml +++ b/.github/workflows/pre-commit.yml @@ -9,7 +9,7 @@ on: jobs: # Call the workflow in the XRPLF/actions repo that runs the pre-commit hooks. run-hooks: - uses: XRPLF/actions/.github/workflows/pre-commit.yml@34790936fae4c6c751f62ec8c06696f9c1a5753a + uses: XRPLF/actions/.github/workflows/pre-commit.yml@5ca417783f0312ab26d6f48b85c78edf1de99bbd with: runs_on: ubuntu-latest container: '{ "image": "ghcr.io/xrplf/ci/tools-rippled-pre-commit:sha-a8c7be1" }' diff --git a/.github/workflows/reusable-build-test-config.yml b/.github/workflows/reusable-build-test-config.yml index 575984162e..fc80bbd216 100644 --- a/.github/workflows/reusable-build-test-config.yml +++ b/.github/workflows/reusable-build-test-config.yml @@ -100,9 +100,9 @@ jobs: uses: actions/checkout@08eba0b27e820071cde6df949e0beb9ba4906955 # v4.3.0 - name: Prepare runner - uses: XRPLF/actions/prepare-runner@2ece4ec6ab7de266859a6f053571425b2bd684b6 + uses: XRPLF/actions/prepare-runner@65da1c59e81965eeb257caa3587b9d45066fb925 with: - disable_ccache: ${{ !inputs.ccache_enabled }} + enable_ccache: ${{ inputs.ccache_enabled }} - name: Set ccache log file if: ${{ inputs.ccache_enabled && runner.debug == '1' }} diff --git a/.github/workflows/upload-conan-deps.yml b/.github/workflows/upload-conan-deps.yml index 8a9993d37a..55a9ab8864 100644 --- a/.github/workflows/upload-conan-deps.yml +++ b/.github/workflows/upload-conan-deps.yml @@ -70,9 +70,9 @@ jobs: uses: actions/checkout@08eba0b27e820071cde6df949e0beb9ba4906955 # v4.3.0 - name: Prepare runner - uses: XRPLF/actions/prepare-runner@2ece4ec6ab7de266859a6f053571425b2bd684b6 + uses: XRPLF/actions/prepare-runner@65da1c59e81965eeb257caa3587b9d45066fb925 with: - disable_ccache: true + enable_ccache: false - name: Print build environment uses: ./.github/actions/print-env From d5c53dcfd2cd4e26d14604ebff8d67d9e16d3211 Mon Sep 17 00:00:00 2001 From: pwang200 <354723+pwang200@users.noreply.github.com> Date: Thu, 8 Jan 2026 16:14:49 -0500 Subject: [PATCH 02/11] fix Uninitialized import entries lead to undefined behavior During WASM Instantiation --- src/xrpld/app/wasm/detail/WasmiVM.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/xrpld/app/wasm/detail/WasmiVM.cpp b/src/xrpld/app/wasm/detail/WasmiVM.cpp index 82c502f63f..40ec5f81b6 100644 --- a/src/xrpld/app/wasm/detail/WasmiVM.cpp +++ b/src/xrpld/app/wasm/detail/WasmiVM.cpp @@ -415,6 +415,7 @@ ModuleWrapper::buildImports(StorePtr& s, ImportVec const& imports) "/" + std::to_string(importTypes.vec_.size), nullptr, j_); + throw std::runtime_error("Missing imports"); } return wimports; From 9ed60b45f8547078aec8a339a209b84d4ae447bb Mon Sep 17 00:00:00 2001 From: pwang200 <354723+pwang200@users.noreply.github.com> Date: Thu, 8 Jan 2026 16:15:36 -0500 Subject: [PATCH 03/11] section corruption unit tests --- src/test/app/Wasm_test.cpp | 24 +++++ src/test/app/wasm_fixtures/fixtures.cpp | 116 ++++++++++++++++++++++++ src/test/app/wasm_fixtures/fixtures.h | 11 +++ 3 files changed, 151 insertions(+) diff --git a/src/test/app/Wasm_test.cpp b/src/test/app/Wasm_test.cpp index 59961fded8..367afbafd0 100644 --- a/src/test/app/Wasm_test.cpp +++ b/src/test/app/Wasm_test.cpp @@ -828,6 +828,28 @@ struct Wasm_test : public beast::unit_test::suite BEAST_EXPECT(runFinishFunction(wasiPrintHex).has_value() == false); } + void + testWasmSectionCorruption() + { + testcase("Wasm Section Corruption tests"); + BEAST_EXPECT(runFinishFunction(badMagicNumberHex).has_value() == false); + BEAST_EXPECT( + runFinishFunction(badVersionNumberHex).has_value() == false); + BEAST_EXPECT(runFinishFunction(lyingHeaderHex).has_value() == false); + BEAST_EXPECT( + runFinishFunction(neverEndingNumberHex).has_value() == false); + BEAST_EXPECT(runFinishFunction(vectorLieHex).has_value() == false); + BEAST_EXPECT( + runFinishFunction(sectionOrderingHex).has_value() == false); + BEAST_EXPECT(runFinishFunction(ghostPayloadHex).has_value() == false); + BEAST_EXPECT( + runFinishFunction(junkAfterSectionHex).has_value() == false); + BEAST_EXPECT( + runFinishFunction(invalidSectionIdHex).has_value() == false); + BEAST_EXPECT( + runFinishFunction(localVariableBombHex).has_value() == false); + } + void run() override { @@ -854,6 +876,8 @@ struct Wasm_test : public beast::unit_test::suite testWasmProposal(); testWasmTrap(); testWasmWasi(); + testWasmSectionCorruption(); + // perfTest(); } }; diff --git a/src/test/app/wasm_fixtures/fixtures.cpp b/src/test/app/wasm_fixtures/fixtures.cpp index 20b98f8647..aa30cf08e7 100644 --- a/src/test/app/wasm_fixtures/fixtures.cpp +++ b/src/test/app/wasm_fixtures/fixtures.cpp @@ -1209,6 +1209,122 @@ extern std::string const wasiPrintHex = "45047f410105417f0b0b0b1e030041100b0648656c6c6f0a0041000b04100000000041040b" "0406000000"; +// The following several wasm hex strings are for testing wasm section +// corruption cases. They are illegal hence do not have corresponding +// rust or wat sources. +// Wasm code magic number is "0061736d", and the only valid version is 1. + +extern std::string const badMagicNumberHex = "1061736d01000000"; +extern std::string const badVersionNumberHex = "0061736d02000000"; + +// Corruption Test: lyingHeader +// Scenario: A section declares it is 2GB long, but the file ends immediately. +// Attack: Buffer pre-allocation DoS (OOM). +// # Magic (00 61 73 6d) + Version (01 00 00 00) +// data = b'\x00\x61\x73\x6d\x01\x00\x00\x00' +// # Type Section (ID 1) +// # Size: LEB128 encoded 2GB (0x80 0x80 0x80 0x80 0x08) +// data += b'\x01\x80\x80\x80\x80\x08' +extern std::string const lyingHeaderHex = "0061736d01000000018080808008"; + +// Corruption Test: neverEndingNumber +// Scenario: An LEB128 integer that never has a stop bit (byte < 0x80). +// Attack: Infinite loop in parser or read out of bounds. +// data = b'\x00\x61\x73\x6d\x01\x00\x00\x00' +// # Type Section (ID 1), Size 5 +// data += b'\x01\x05' +// # Vector count: Infinite stream of 0x80 (100 bytes) +// data += b'\x80' * 100 +extern std::string const neverEndingNumberHex = + "0061736d010000000105808080808080808080808080808080808080808080808080808080" + "80808080808080808080808080808080808080808080808080808080808080808080808080" + "808080808080808080808080808080808080808080808080808080808080808080808080"; + +// Corruption Test: vectorLie +// Scenario: A vector declares it has 4 billion items, but provides none. +// Attack: Vector pre-allocation DoS (OOM). +// data = b'\x00\x61\x73\x6d\x01\x00\x00\x00' +// # Type Section (ID 1) +// # Size 5 (just enough for the count bytes) +// data += b'\x01\x05' +// # Vector Count: 0xFF 0xFF 0xFF 0xFF 0x0F (4,294,967,295 items) +// data += b'\xff\xff\xff\xff\x0f' +// # No actual items follow... +extern std::string const vectorLieHex = "0061736d010000000105ffffffff0f"; + +// Corruption Test: sectionOrdering +// Scenario: Sections appear out of order +// (Code section before Function section). +// Attack: Parser state confusion / potential null pointer deref. +// data = b'\x00\x61\x73\x6d\x01\x00\x00\x00' +// # Code Section (ID 10) - usually last +// # Size 2, Count 0 +// data += b'\x0a\x02\x00\x0b' +// # Function Section (ID 3) - usually 3rd +// data += b'\x03\x02\x00\x00' +extern std::string const sectionOrderingHex = + "0061736d010000000a02000b03020000"; + +// Corruption Test: ghostPayload +// Scenario: Valid headers, but file is truncated in the middle of a payload. +// Attack: Read out of bounds panic. +// data = b'\x00\x61\x73\x6d\x01\x00\x00\x00' +// # Type Section (ID 1), Size 10 +// data += b'\x01\x0a' +// # Content: Count 1 +// data += b'\x01' +// # Start of a type definition (0x60 = func) +// data += b'\x60' +// # File ends abruptly here (missing params/results) +extern std::string const ghostPayloadHex = "0061736d01000000010a0160"; + +// Corruption Test: junkAfterSection +// Scenario: Section declares size X, but logical content finishes at X-5. +// Attack: Validation bypass if parser stops early, +// or panic if strict check missing. +// data = b'\x00\x61\x73\x6d\x01\x00\x00\x00' +// # Type Section (ID 1), Size 10 bytes +// data += b'\x01\x0a' +// # Real content: Count 1, (func -> void) = 4 bytes +// # \x01 (count) \x60 (func) \x00 (0 params) \x00 (0 results) +// data += b'\x01\x60\x00\x00' +// # Remaining 6 bytes are junk padding within the section size +// data += b'\x00' * 6 +extern std::string const junkAfterSectionHex = + "0061736d01000000010a01600000000000000000"; + +// Corruption Test: invalidSectionId +// Scenario: A section ID that doesn't exist (0xFF). +// Attack: Default case handling / unhandled enum variant. +// data = b'\x00\x61\x73\x6d\x01\x00\x00\x00' +// # Section ID 0xFF, Size 1 +// data += b'\xff\x01\x00' +extern std::string const invalidSectionIdHex = "0061736d01000000ff0100"; + +// Corruption Test: localVariableBomb +// Scenario: A function declares 4 billion local variables. +// Attack: Stack Overflow / OOM during function init (memset). +// data = b'\x00\x61\x73\x6d\x01\x00\x00\x00' +// # 1. Type Section: (func) -> () +// data += b'\x01\x04\x01\x60\x00\x00' +// # 3. Function Section: 1 function of type 0 +// data += b'\x03\x02\x01\x00' +// # 10. Code Section +// # ID 10, Size 15 (estimated), Count 1 +// data += b'\x0a\x0f\x01' +// # Function Body Size: 13 bytes +// data += b'\x0d' +// # Local Declarations Count: 1 entry +// data += b'\x01' +// # The Bomb: 4,294,967,295 locals of type i32 +// # Count: 0xFF 0xFF 0xFF 0xFF 0x0F +// # Type: 0x7F (i32) +// data += b'\xff\xff\xff\xff\x0f\x7f' +// # Instruction: end (0x0b) +// data += b'\x0b' +extern std::string const localVariableBombHex = + "0061736d01000000010401600000030201000a0f010d01ffffffff0f7f0b"; + extern std::string const infiniteLoopWasmHex = "0061736d010000000108026000006000017f030302000105030100020638097f004180080b" "7f004180080b7f004180080b7f00418088040b7f004180080b7f00418088040b7f00418080" diff --git a/src/test/app/wasm_fixtures/fixtures.h b/src/test/app/wasm_fixtures/fixtures.h index 431ee612e7..ef1ecd9de5 100644 --- a/src/test/app/wasm_fixtures/fixtures.h +++ b/src/test/app/wasm_fixtures/fixtures.h @@ -65,4 +65,15 @@ extern std::string const trapFuncSigMismatchHex; extern std::string const wasiGetTimeHex; extern std::string const wasiPrintHex; +extern std::string const badMagicNumberHex; +extern std::string const badVersionNumberHex; +extern std::string const lyingHeaderHex; +extern std::string const neverEndingNumberHex; +extern std::string const vectorLieHex; +extern std::string const sectionOrderingHex; +extern std::string const ghostPayloadHex; +extern std::string const junkAfterSectionHex; +extern std::string const invalidSectionIdHex; +extern std::string const localVariableBombHex; + extern std::string const infiniteLoopWasmHex; From 91f3d51f3d296ffa41ae70ea23578e3adfefaf85 Mon Sep 17 00:00:00 2001 From: pwang200 <354723+pwang200@users.noreply.github.com> Date: Fri, 9 Jan 2026 11:38:54 -0500 Subject: [PATCH 04/11] fix start function loop --- src/test/app/Wasm_test.cpp | 33 +++++++++++++++++++ src/test/app/wasm_fixtures/fixtures.cpp | 4 +++ src/test/app/wasm_fixtures/fixtures.h | 1 + src/test/app/wasm_fixtures/wat/start_loop.wat | 22 +++++++++++++ src/xrpld/app/wasm/WasmiVM.h | 4 +++ src/xrpld/app/wasm/detail/WasmiVM.cpp | 33 +++++++++++++++---- 6 files changed, 91 insertions(+), 6 deletions(-) create mode 100644 src/test/app/wasm_fixtures/wat/start_loop.wat diff --git a/src/test/app/Wasm_test.cpp b/src/test/app/Wasm_test.cpp index 367afbafd0..dba390c93e 100644 --- a/src/test/app/Wasm_test.cpp +++ b/src/test/app/Wasm_test.cpp @@ -850,6 +850,38 @@ struct Wasm_test : public beast::unit_test::suite runFinishFunction(localVariableBombHex).has_value() == false); } + void + testStartFunctionLoop() + { + testcase("infinite loop in start function"); + + using namespace test::jtx; + Env env(*this); + + auto wasmStr = boost::algorithm::unhex(startLoopHex); + Bytes wasm(wasmStr.begin(), wasmStr.end()); + TestLedgerDataProvider ledgerDataProvider(env); + ImportVec imports; + + auto& engine = WasmEngine::instance(); + auto checkRes = engine.check( + wasm, "finish", {}, imports, &ledgerDataProvider, env.journal); + BEAST_EXPECTS( + checkRes == tesSUCCESS, std::to_string(TERtoInt(checkRes))); + + auto re = engine.run( + wasm, + ESCROW_FUNCTION_NAME, + {}, + imports, + &ledgerDataProvider, + 1'000'000, + env.journal); + BEAST_EXPECTS( + re.error() == tecFAILED_PROCESSING, + std::to_string(TERtoInt(re.error()))); + } + void run() override { @@ -878,6 +910,7 @@ struct Wasm_test : public beast::unit_test::suite testWasmWasi(); testWasmSectionCorruption(); + testStartFunctionLoop(); // perfTest(); } }; diff --git a/src/test/app/wasm_fixtures/fixtures.cpp b/src/test/app/wasm_fixtures/fixtures.cpp index aa30cf08e7..740638ba61 100644 --- a/src/test/app/wasm_fixtures/fixtures.cpp +++ b/src/test/app/wasm_fixtures/fixtures.cpp @@ -1340,3 +1340,7 @@ extern std::string const infiniteLoopWasmHex = "303861373930636664623432626432343732302900490f7461726765745f66656174757265" "73042b0f6d757461626c652d676c6f62616c732b087369676e2d6578742b0f726566657265" "6e63652d74797065732b0a6d756c746976616c7565"; + +extern std::string const startLoopHex = + "0061736d010000000108026000006000017f03030200010712020573746172740000066669" + "6e69736800010801000a0e02070003400c000b0b040041010b"; diff --git a/src/test/app/wasm_fixtures/fixtures.h b/src/test/app/wasm_fixtures/fixtures.h index ef1ecd9de5..7da8fcb4dc 100644 --- a/src/test/app/wasm_fixtures/fixtures.h +++ b/src/test/app/wasm_fixtures/fixtures.h @@ -77,3 +77,4 @@ extern std::string const invalidSectionIdHex; extern std::string const localVariableBombHex; extern std::string const infiniteLoopWasmHex; +extern std::string const startLoopHex; diff --git a/src/test/app/wasm_fixtures/wat/start_loop.wat b/src/test/app/wasm_fixtures/wat/start_loop.wat new file mode 100644 index 0000000000..c00d173ea4 --- /dev/null +++ b/src/test/app/wasm_fixtures/wat/start_loop.wat @@ -0,0 +1,22 @@ +(module + ;; Function 1: The Infinite Loop + (func $run_forever + (loop $infinite + br $infinite + ) + ) + + ;; Function 2: Finish + (func $finish (result i32) + i32.const 1 + ) + + ;; 1. EXPORT the functions (optional, if you want to call them later) + (export "start" (func $run_forever)) + (export "finish" (func $finish)) + + ;; 2. The special start section + ;; This tells the VM: "Run function $run_forever immediately + ;; when this module is instantiated." + (start $run_forever) +) diff --git a/src/xrpld/app/wasm/WasmiVM.h b/src/xrpld/app/wasm/WasmiVM.h index 06e18ef29e..c39b577c46 100644 --- a/src/xrpld/app/wasm/WasmiVM.h +++ b/src/xrpld/app/wasm/WasmiVM.h @@ -179,6 +179,10 @@ public: FuncInfo getFunc(std::string_view funcName) const; + + wasm_functype_t* + getFuncType(std::string_view funcName) const; + wmem getMem() const; diff --git a/src/xrpld/app/wasm/detail/WasmiVM.cpp b/src/xrpld/app/wasm/detail/WasmiVM.cpp index 40ec5f81b6..016adeac5a 100644 --- a/src/xrpld/app/wasm/detail/WasmiVM.cpp +++ b/src/xrpld/app/wasm/detail/WasmiVM.cpp @@ -249,9 +249,9 @@ ModuleWrapper::ModuleWrapper( : module_(init(s, wasmBin, j)), j_(j) { wasm_module_exports(module_.get(), &exportTypes_.vec_); + auto wimports = buildImports(s, imports); if (instantiate) { - auto wimports = buildImports(s, imports); addInstance(s, wimports); } } @@ -427,6 +427,26 @@ ModuleWrapper::getFunc(std::string_view funcName) const return instanceWrap_.getFunc(funcName, exportTypes_); } +wasm_functype_t* +ModuleWrapper::getFuncType(std::string_view funcName) const +{ + for (size_t i = 0; i < exportTypes_.vec_.size; i++) + { + auto const* exp_type(exportTypes_.vec_.data[i]); + wasm_name_t const* name = wasm_exporttype_name(exp_type); + wasm_externtype_t const* exn_type = wasm_exporttype_type(exp_type); + if (wasm_externtype_kind(exn_type) == WASM_EXTERN_FUNC && + funcName == std::string_view(name->data, name->size)) + { + return wasm_externtype_as_functype( + const_cast(exn_type)); + } + } + + throw std::runtime_error( + "can't find function <" + std::string(funcName) + ">"); +} + wmem ModuleWrapper::getMem() const { @@ -889,13 +909,14 @@ WasmiEngine::checkHlp( if (wasmCode.empty()) throw std::runtime_error("empty nodule"); - int const m = addModule(wasmCode, true, -1, imports); - if ((m < 0) || !moduleWrap_ || !moduleWrap_->instanceWrap_) - throw std::runtime_error("no instance"); // LCOV_EXCL_LINE + int const m = addModule(wasmCode, false, -1, imports); + if ((m < 0) || !moduleWrap_) + throw std::runtime_error("no module"); // LCOV_EXCL_LINE // Looking for a func and compare parameter types - auto const f = getFunc(!funcName.empty() ? funcName : "_start"); - auto const* ftp = wasm_functype_params(f.second); + auto const f = + moduleWrap_->getFuncType(!funcName.empty() ? funcName : "_start"); + auto const* ftp = wasm_functype_params(f); auto const p = convertParams(params); if (int const comp = compareParamTypes(ftp, p); comp >= 0) From c24a6041f7fa8509a584de0c82c439b43197eddf Mon Sep 17 00:00:00 2001 From: oncecelll Date: Sat, 10 Jan 2026 02:15:05 +0800 Subject: [PATCH 05/11] docs: Fix minor spelling issues in comments (#6194) --- src/libxrpl/protocol/SecretKey.cpp | 4 ++-- src/test/app/HashRouter_test.cpp | 2 +- src/test/app/MPToken_test.cpp | 4 ++-- src/test/app/MultiSign_test.cpp | 2 +- src/test/basics/IntrusiveShared_test.cpp | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/libxrpl/protocol/SecretKey.cpp b/src/libxrpl/protocol/SecretKey.cpp index 88404a88a5..2507269407 100644 --- a/src/libxrpl/protocol/SecretKey.cpp +++ b/src/libxrpl/protocol/SecretKey.cpp @@ -77,7 +77,7 @@ deriveDeterministicRootKey(Seed const& seed) std::array buf; std::copy(seed.begin(), seed.end(), buf.begin()); - // The odds that this loop executes more than once are neglible + // The odds that this loop executes more than once are negligible // but *just* in case someone managed to generate a key that required // more iterations loop a few times. for (std::uint32_t seq = 0; seq != 128; ++seq) @@ -137,7 +137,7 @@ private: std::copy(generator_.begin(), generator_.end(), buf.begin()); copy_uint32(buf.data() + 33, seq); - // The odds that this loop executes more than once are neglible + // The odds that this loop executes more than once are negligible // but we impose a maximum limit just in case. for (std::uint32_t subseq = 0; subseq != 128; ++subseq) { diff --git a/src/test/app/HashRouter_test.cpp b/src/test/app/HashRouter_test.cpp index c428917fdc..2d1d37c3e3 100644 --- a/src/test/app/HashRouter_test.cpp +++ b/src/test/app/HashRouter_test.cpp @@ -349,7 +349,7 @@ class HashRouter_test : public beast::unit_test::suite h.set("hold_time", "alice"); h.set("relay_time", "bob"); auto const setup = setup_HashRouter(cfg); - // The set function ignores values that don't covert, so the + // The set function ignores values that don't convert, so the // defaults are left unchanged BEAST_EXPECT(setup.holdTime == 300s); BEAST_EXPECT(setup.relayTime == 30s); diff --git a/src/test/app/MPToken_test.cpp b/src/test/app/MPToken_test.cpp index ed6d861ffb..747f78ef6b 100644 --- a/src/test/app/MPToken_test.cpp +++ b/src/test/app/MPToken_test.cpp @@ -2651,7 +2651,7 @@ class MPToken_test : public beast::unit_test::suite STAmount const amt3{asset3, 10'000}; { - testcase("Test STAmount MPT arithmetics"); + testcase("Test STAmount MPT arithmetic"); using namespace std::string_literals; STAmount res = multiply(amt1, amt2, asset3); BEAST_EXPECT(res == amt3); @@ -2688,7 +2688,7 @@ class MPToken_test : public beast::unit_test::suite } { - testcase("Test MPTAmount arithmetics"); + testcase("Test MPTAmount arithmetic"); MPTAmount mptAmt1{100}; MPTAmount const mptAmt2{100}; BEAST_EXPECT((mptAmt1 += mptAmt2) == MPTAmount{200}); diff --git a/src/test/app/MultiSign_test.cpp b/src/test/app/MultiSign_test.cpp index 6950286b52..5c5404c17e 100644 --- a/src/test/app/MultiSign_test.cpp +++ b/src/test/app/MultiSign_test.cpp @@ -708,7 +708,7 @@ public: void testHeterogeneousSigners(FeatureBitset features) { - testcase("Heterogenous Signers"); + testcase("Heterogeneous Signers"); using namespace jtx; Env env{*this, features}; diff --git a/src/test/basics/IntrusiveShared_test.cpp b/src/test/basics/IntrusiveShared_test.cpp index b77325efa9..500b6e7e39 100644 --- a/src/test/basics/IntrusiveShared_test.cpp +++ b/src/test/basics/IntrusiveShared_test.cpp @@ -396,7 +396,7 @@ public: // This checks that partialDelete has run to completion // before the destructor is called. A sleep is inserted // inside the partial delete to make sure the destructor is - // given an opportunity to run durring partial delete. + // given an opportunity to run during partial delete. BEAST_EXPECT(cur == partiallyDeleted); } if (next == partiallyDeletedStarted) From fc0072383699cf9718af35a22e46df1bad7799bb Mon Sep 17 00:00:00 2001 From: Zhanibek Bakin <50952098+janibakin@users.noreply.github.com> Date: Fri, 9 Jan 2026 23:37:55 +0500 Subject: [PATCH 06/11] fix: Truncate thread name to 15 chars on Linux (#5758) This change: * Truncates thread names if more than 15 chars with `snprintf`. * Adds warnings for truncated thread names if `-DTRUNCATED_THREAD_NAME_LOGS=ON`. * Add a static assert for string literals to stop compiling if > 15 chars. * Shortens `Resource::Manager` to `Resource::Mngr` to fix the static assert failure. * Updates `CurrentThreadName_test` unit test specifically for Linux to verify truncation. --- cmake/XrplSettings.cmake | 15 ++++ include/xrpl/beast/core/CurrentThreadName.h | 27 +++++++ src/libxrpl/beast/core/CurrentThreadName.cpp | 24 +++++- src/libxrpl/resource/ResourceManager.cpp | 2 +- .../beast/beast_CurrentThreadName_test.cpp | 74 ++++++++++++++----- 5 files changed, 121 insertions(+), 21 deletions(-) diff --git a/cmake/XrplSettings.cmake b/cmake/XrplSettings.cmake index a16513afc5..c3f013c575 100644 --- a/cmake/XrplSettings.cmake +++ b/cmake/XrplSettings.cmake @@ -68,6 +68,21 @@ if(is_linux) option(perf "Enables flags that assist with perf recording" OFF) option(use_gold "enables detection of gold (binutils) linker" ON) option(use_mold "enables detection of mold (binutils) linker" ON) + # Set a default value for the log flag based on the build type. + # This provides a sensible default (on for debug, off for release) + # while still allowing the user to override it for any build. + if(CMAKE_BUILD_TYPE STREQUAL "Debug") + set(TRUNCATED_LOGS_DEFAULT ON) + else() + set(TRUNCATED_LOGS_DEFAULT OFF) + endif() + option(TRUNCATED_THREAD_NAME_LOGS + "Show warnings about truncated thread names on Linux." + ${TRUNCATED_LOGS_DEFAULT} + ) + if(TRUNCATED_THREAD_NAME_LOGS) + add_compile_definitions(TRUNCATED_THREAD_NAME_LOGS) + endif() else() # we are not ready to allow shared-libs on windows because it would require # export declarations. On macos it's more feasible, but static openssl diff --git a/include/xrpl/beast/core/CurrentThreadName.h b/include/xrpl/beast/core/CurrentThreadName.h index 8e9d58b649..703246a76a 100644 --- a/include/xrpl/beast/core/CurrentThreadName.h +++ b/include/xrpl/beast/core/CurrentThreadName.h @@ -5,6 +5,8 @@ #ifndef BEAST_CORE_CURRENT_THREAD_NAME_H_INCLUDED #define BEAST_CORE_CURRENT_THREAD_NAME_H_INCLUDED +#include + #include #include @@ -16,6 +18,31 @@ namespace beast { void setCurrentThreadName(std::string_view newThreadName); +#if BOOST_OS_LINUX + +// On Linux, thread names are limited to 16 bytes including the null terminator. +// Maximum number of characters is therefore 15. +constexpr std::size_t maxThreadNameLength = 15; + +/** Sets the name of the caller thread with compile-time size checking. + @tparam N The size of the string literal including null terminator + @param newThreadName A string literal to set as the thread name + + This template overload enforces that thread names are at most 16 characters + (including null terminator) at compile time, matching Linux's limit. +*/ +template +void +setCurrentThreadName(char const (&newThreadName)[N]) +{ + static_assert( + N <= maxThreadNameLength + 1, + "Thread name cannot exceed 15 characters"); + + setCurrentThreadName(std::string_view(newThreadName, N - 1)); +} +#endif + /** Returns the name of the caller thread. The name returned is the name as set by a call to setCurrentThreadName(). diff --git a/src/libxrpl/beast/core/CurrentThreadName.cpp b/src/libxrpl/beast/core/CurrentThreadName.cpp index 42dbb062b4..e8f7b629a7 100644 --- a/src/libxrpl/beast/core/CurrentThreadName.cpp +++ b/src/libxrpl/beast/core/CurrentThreadName.cpp @@ -1,7 +1,5 @@ #include -#include - #include #include @@ -73,12 +71,32 @@ setCurrentThreadNameImpl(std::string_view name) #if BOOST_OS_LINUX #include +#include + namespace beast::detail { inline void setCurrentThreadNameImpl(std::string_view name) { - pthread_setname_np(pthread_self(), name.data()); + // truncate and set the thread name. + char boundedName[maxThreadNameLength + 1]; + std::snprintf( + boundedName, + sizeof(boundedName), + "%.*s", + static_cast(maxThreadNameLength), + name.data()); + + pthread_setname_np(pthread_self(), boundedName); + +#ifdef TRUNCATED_THREAD_NAME_LOGS + if (name.size() > maxThreadNameLength) + { + std::cerr << "WARNING: Thread name \"" << name << "\" (length " + << name.size() << ") exceeds maximum of " + << maxThreadNameLength << " characters on Linux.\n"; + } +#endif } } // namespace beast::detail diff --git a/src/libxrpl/resource/ResourceManager.cpp b/src/libxrpl/resource/ResourceManager.cpp index 8582836611..15d31a558e 100644 --- a/src/libxrpl/resource/ResourceManager.cpp +++ b/src/libxrpl/resource/ResourceManager.cpp @@ -140,7 +140,7 @@ private: void run() { - beast::setCurrentThreadName("Resource::Manager"); + beast::setCurrentThreadName("Resource::Mngr"); for (;;) { logic_.periodicActivity(); diff --git a/src/test/beast/beast_CurrentThreadName_test.cpp b/src/test/beast/beast_CurrentThreadName_test.cpp index 3d33ecb602..dc12883a63 100644 --- a/src/test/beast/beast_CurrentThreadName_test.cpp +++ b/src/test/beast/beast_CurrentThreadName_test.cpp @@ -1,6 +1,8 @@ #include #include +#include + #include namespace xrpl { @@ -37,33 +39,71 @@ private: if (beast::getCurrentThreadName() == myName) *state = 2; } +#if BOOST_OS_LINUX + // Helper function to test a specific name. + // It creates a thread, sets the name, and checks if the OS-level + // name matches the expected (potentially truncated) name. + void + testName(std::string const& nameToSet, std::string const& expectedName) + { + std::thread t([&] { + beast::setCurrentThreadName(nameToSet); + + // Initialize buffer to be safe. + char actualName[beast::maxThreadNameLength + 1] = {}; + pthread_getname_np(pthread_self(), actualName, sizeof(actualName)); + + BEAST_EXPECT(std::string(actualName) == expectedName); + }); + t.join(); + } +#endif public: void run() override { - // Make two different threads with two different names. Make sure - // that the expected thread names are still there when the thread - // exits. - std::atomic stop{false}; + // Make two different threads with two different names. + // Make sure that the expected thread names are still there + // when the thread exits. + { + std::atomic stop{false}; - std::atomic stateA{0}; - std::thread tA(exerciseName, "tA", &stop, &stateA); + std::atomic stateA{0}; + std::thread tA(exerciseName, "tA", &stop, &stateA); - std::atomic stateB{0}; - std::thread tB(exerciseName, "tB", &stop, &stateB); + std::atomic stateB{0}; + std::thread tB(exerciseName, "tB", &stop, &stateB); - // Wait until both threads have set their names. - while (stateA == 0 || stateB == 0) - ; + // Wait until both threads have set their names. + while (stateA == 0 || stateB == 0) + ; - stop = true; - tA.join(); - tB.join(); + stop = true; + tA.join(); + tB.join(); - // Both threads should still have the expected name when they exit. - BEAST_EXPECT(stateA == 2); - BEAST_EXPECT(stateB == 2); + // Both threads should still have the expected name when they exit. + BEAST_EXPECT(stateA == 2); + BEAST_EXPECT(stateB == 2); + } +#if BOOST_OS_LINUX + // On Linux, verify that thread names longer than 15 characters + // are truncated to 15 characters (the 16th character is reserved for + // the null terminator). + { + testName( + "123456789012345", + "123456789012345"); // 15 chars, no truncation + testName( + "1234567890123456", "123456789012345"); // 16 chars, truncated + testName( + "ThisIsAVeryLongThreadNameExceedingLimit", + "ThisIsAVeryLong"); // 39 chars, truncated + testName("", ""); // empty name + testName("short", "short"); // short name, no truncation + } +#endif } }; From 6ab15f8377019b8b29fd0a3035d766040a6ca695 Mon Sep 17 00:00:00 2001 From: Olek <115580134+oleks-rip@users.noreply.github.com> Date: Fri, 9 Jan 2026 14:49:09 -0500 Subject: [PATCH 07/11] Add checks to allocate (#6185) --- src/test/app/Wasm_test.cpp | 77 +++++++++++++++++++++++++ src/test/app/wasm_fixtures/bad_alloc.c | 27 +++++++++ src/test/app/wasm_fixtures/fixtures.cpp | 18 ++++++ src/test/app/wasm_fixtures/fixtures.h | 2 + src/xrpld/app/wasm/detail/WasmiVM.cpp | 21 +++++-- 5 files changed, 140 insertions(+), 5 deletions(-) create mode 100644 src/test/app/wasm_fixtures/bad_alloc.c diff --git a/src/test/app/Wasm_test.cpp b/src/test/app/Wasm_test.cpp index dba390c93e..95986f5a76 100644 --- a/src/test/app/Wasm_test.cpp +++ b/src/test/app/Wasm_test.cpp @@ -882,6 +882,81 @@ struct Wasm_test : public beast::unit_test::suite std::to_string(TERtoInt(re.error()))); } + void + testBadAlloc() + { + testcase("Wasm Bad Alloc"); + + // bad_alloc.c + auto wasmStr = boost::algorithm::unhex(badAllocHex); + Bytes wasm(wasmStr.begin(), wasmStr.end()); + + using namespace test::jtx; + + Env env{*this}; + TestLedgerDataProvider hf(env); + + // ImportVec imports; + uint8_t buf1[8] = {7, 8, 9, 10, 11, 12, 13, 14}; + { // forged "allocate" return valid address + std::vector params = { + {.type = WT_U8V, + .of = {.u8v = {.d = buf1, .sz = sizeof(buf1)}}}}; + auto& engine = WasmEngine::instance(); + + auto re = engine.run( + wasm, "test", params, {}, &hf, 1'000'000, env.journal); + if (BEAST_EXPECT(re)) + { + BEAST_EXPECTS(re->result == 7, std::to_string(re->result)); + BEAST_EXPECTS(re->cost == 10, std::to_string(re->result)); + } + } + + { // return 0 whithout calling wasm + std::vector params = { + {.type = WT_U8V, .of = {.u8v = {.d = buf1, .sz = 0}}}}; + auto& engine = WasmEngine::instance(); + auto re = engine.run( + wasm, "test", params, {}, &hf, 1'000'000, env.journal); + BEAST_EXPECT(!re) && + BEAST_EXPECT(re.error() == tecFAILED_PROCESSING); + } + + { // forged "allocate" return 8Mb (which is more than memory limit) + std::vector params = { + {.type = WT_U8V, .of = {.u8v = {.d = buf1, .sz = 1}}}}; + auto& engine = WasmEngine::instance(); + auto re = engine.run( + wasm, "test", params, {}, &hf, 1'000'000, env.journal); + BEAST_EXPECT(!re) && + BEAST_EXPECT(re.error() == tecFAILED_PROCESSING); + } + + { // forged "allocate" return 0 + std::vector params = { + {.type = WT_U8V, .of = {.u8v = {.d = buf1, .sz = 2}}}}; + auto& engine = WasmEngine::instance(); + auto re = engine.run( + wasm, "test", params, {}, &hf, 1'000'000, env.journal); + BEAST_EXPECT(!re) && + BEAST_EXPECT(re.error() == tecFAILED_PROCESSING); + } + + { // forged "allocate" return -1 + std::vector params = { + {.type = WT_U8V, .of = {.u8v = {.d = buf1, .sz = 3}}}}; + auto& engine = WasmEngine::instance(); + auto re = engine.run( + wasm, "test", params, {}, &hf, 1'000'000, env.journal); + + BEAST_EXPECT(!re) && + BEAST_EXPECT(re.error() == tecFAILED_PROCESSING); + } + + env.close(); + } + void run() override { @@ -911,6 +986,8 @@ struct Wasm_test : public beast::unit_test::suite testWasmSectionCorruption(); testStartFunctionLoop(); + testBadAlloc(); + // perfTest(); } }; diff --git a/src/test/app/wasm_fixtures/bad_alloc.c b/src/test/app/wasm_fixtures/bad_alloc.c new file mode 100644 index 0000000000..9a0233e8a5 --- /dev/null +++ b/src/test/app/wasm_fixtures/bad_alloc.c @@ -0,0 +1,27 @@ +#include + +char buf[1024]; + +void* +allocate(int sz) +{ + if (!sz) + return 0; + + if (sz == 1) + return ((void*)(8 * 1024 * 1024)); + if (sz == 2) + return 0; + if (sz == 3) + return ((void*)(0xFFFFFFFF)); + + return &buf[sz]; +} + +int32_t +test(char* p, int32_t sz) +{ + if (!sz) + return 0; + return p[0]; +} diff --git a/src/test/app/wasm_fixtures/fixtures.cpp b/src/test/app/wasm_fixtures/fixtures.cpp index 740638ba61..97143f1f61 100644 --- a/src/test/app/wasm_fixtures/fixtures.cpp +++ b/src/test/app/wasm_fixtures/fixtures.cpp @@ -1344,3 +1344,21 @@ extern std::string const infiniteLoopWasmHex = extern std::string const startLoopHex = "0061736d010000000108026000006000017f03030200010712020573746172740000066669" "6e69736800010801000a0e02070003400c000b0b040041010b"; + +extern std::string const badAllocHex = + "0061736d01000000010f0360000060017f017f60027f7f017f030403000102050301000206" + "3e0a7f004180080b7f004180080b7f004180100b7f004180100b7f00418090040b7f004180" + "080b7f00418090040b7f00418080080b7f0041000b7f0041010b07b9010e066d656d6f7279" + "0200115f5f7761736d5f63616c6c5f63746f7273000008616c6c6f63617465000103627566" + "0300047465737400020c5f5f64736f5f68616e646c6503010a5f5f646174615f656e640302" + "0b5f5f737461636b5f6c6f7703030c5f5f737461636b5f6869676803040d5f5f676c6f6261" + "6c5f6261736503050b5f5f686561705f6261736503060a5f5f686561705f656e6403070d5f" + "5f6d656d6f72795f6261736503080c5f5f7461626c655f6261736503090a420302000b2c01" + "017f024002400240024020000e0403000301020b41808080040f0b417f0f0b20004180086a" + "21010b20010b1000200145044041000f0b20002c00000b007f0970726f647563657273010c" + "70726f6365737365642d62790105636c616e675f31392e312e352d776173692d73646b2028" + "68747470733a2f2f6769746875622e636f6d2f6c6c766d2f6c6c766d2d70726f6a65637420" + "61623462356132646235383239353861663165653330386137393063666462343262643234" + "3732302900490f7461726765745f6665617475726573042b0f6d757461626c652d676c6f62" + "616c732b087369676e2d6578742b0f7265666572656e63652d74797065732b0a6d756c7469" + "76616c7565"; diff --git a/src/test/app/wasm_fixtures/fixtures.h b/src/test/app/wasm_fixtures/fixtures.h index 7da8fcb4dc..32f71883f8 100644 --- a/src/test/app/wasm_fixtures/fixtures.h +++ b/src/test/app/wasm_fixtures/fixtures.h @@ -78,3 +78,5 @@ extern std::string const localVariableBombHex; extern std::string const infiniteLoopWasmHex; extern std::string const startLoopHex; + +extern std::string const badAllocHex; diff --git a/src/xrpld/app/wasm/detail/WasmiVM.cpp b/src/xrpld/app/wasm/detail/WasmiVM.cpp index 016adeac5a..9d3dded78b 100644 --- a/src/xrpld/app/wasm/detail/WasmiVM.cpp +++ b/src/xrpld/app/wasm/detail/WasmiVM.cpp @@ -951,13 +951,24 @@ WasmiEngine::getRT(int m, int i) int32_t WasmiEngine::allocate(int32_t sz) { - auto res = call<1>(W_ALLOC, static_cast(sz)); - - if (res.f || !res.r.vec_.size || (res.r.vec_.data[0].kind != WASM_I32) || - !res.r.vec_.data[0].of.i32) + if (sz <= 0) throw std::runtime_error( "can't allocate memory, " + std::to_string(sz) + " bytes"); - return res.r.vec_.data[0].of.i32; + + auto res = call<1>(W_ALLOC, static_cast(sz)); + + if (res.f || !res.r.vec_.size || (res.r.vec_.data[0].kind != WASM_I32)) + throw std::runtime_error( + "can't allocate memory, " + std::to_string(sz) + + " bytes"); // LCOV_EXCL_LINE + + int32_t const p = res.r.vec_.data[0].of.i32; + auto const mem = getMem(); + if (p <= 0 || p + sz > mem.s) + throw std::runtime_error( + "invalid memory allocation, " + std::to_string(sz) + " bytes"); + + return p; } wasm_trap_t* From 14467fba5e5652d4976fdea0d312d64e6fb61242 Mon Sep 17 00:00:00 2001 From: Vito Tumas <5780819+Tapanito@users.noreply.github.com> Date: Fri, 9 Jan 2026 20:58:02 +0100 Subject: [PATCH 08/11] VaultClawback: Burn shares of an empty vault (#6120) - Adds a mechanism for the vault owner to burn user shares when the vault is stuck. If the Vault has 0 AssetsAvailable and Total, the owner may submit a VaultClawback to reclaim the worthless fees, and thus allow the Vault to be deleted. The Amount must be left off (unless the owner is the asset issuer), specified as 0 Shares, or specified as the number of Shares held. --- src/test/app/Vault_test.cpp | 589 ++++++++++++++++++++- src/xrpld/app/tx/detail/InvariantCheck.cpp | 77 +-- src/xrpld/app/tx/detail/InvariantCheck.h | 1 + src/xrpld/app/tx/detail/VaultClawback.cpp | 473 +++++++++++------ src/xrpld/app/tx/detail/VaultClawback.h | 8 + 5 files changed, 931 insertions(+), 217 deletions(-) diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index f8d76623fd..d0a1450d6c 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -21,7 +21,6 @@ #include #include #include -#include #include #include #include @@ -940,25 +939,6 @@ class Vault_test : public beast::unit_test::suite } }); - testCase([&](Env& env, - Account const& issuer, - Account const& owner, - Asset const& asset, - Vault& vault) { - testcase("clawback from self"); - - auto [tx, keylet] = vault.create({.owner = owner, .asset = asset}); - - { - auto tx = vault.clawback( - {.issuer = issuer, - .id = keylet.key, - .holder = issuer, - .amount = asset(10)}); - env(tx, ter{temMALFORMED}); - } - }); - testCase([&](Env& env, Account const&, Account const& owner, @@ -1197,11 +1177,13 @@ class Vault_test : public beast::unit_test::suite auto [tx, keylet] = vault.create({.owner = owner, .asset = asset}); + // Preclaim only checks for native assets. + if (asset.native()) { auto tx = vault.clawback( - {.issuer = owner, + {.issuer = issuer, .id = keylet.key, - .holder = issuer, + .holder = owner, .amount = asset(50)}); env(tx, ter(temMALFORMED)); } @@ -1924,8 +1906,20 @@ class Vault_test : public beast::unit_test::suite env.close(); { - auto tx = vault.clawback( - {.issuer = owner, .id = keylet.key, .holder = depositor}); + auto tx = vault.clawback({ + .issuer = depositor, + .id = keylet.key, + .holder = depositor, + }); + env(tx, ter(tecNO_PERMISSION)); + } + + { + auto tx = vault.clawback({ + .issuer = owner, + .id = keylet.key, + .holder = depositor, + }); env(tx, ter(tecNO_PERMISSION)); } }); @@ -2377,6 +2371,15 @@ class Vault_test : public beast::unit_test::suite env(tx, ter(tecNO_AUTH)); } + { + // Cannot clawback if issuer is the holder + tx = vault.clawback( + {.issuer = issuer, + .id = keylet.key, + .holder = issuer, + .amount = asset(800)}); + env(tx, ter(tecNO_PERMISSION)); + } // Clawback works tx = vault.clawback( {.issuer = issuer, @@ -5243,6 +5246,542 @@ class Vault_test : public beast::unit_test::suite }); } + void + testVaultClawbackBurnShares() + { + using namespace test::jtx; + using namespace loanBroker; + using namespace loan; + Env env(*this, beast::severities::kWarning); + + auto const vaultAssetBalance = [&](Keylet const& vaultKeylet) { + auto const sleVault = env.le(vaultKeylet); + BEAST_EXPECT(sleVault != nullptr); + + return std::make_pair( + sleVault->at(sfAssetsAvailable), sleVault->at(sfAssetsTotal)); + }; + + auto const vaultShareBalance = [&](Keylet const& vaultKeylet) { + auto const sleVault = env.le(vaultKeylet); + BEAST_EXPECT(sleVault != nullptr); + + auto const sleIssuance = + env.le(keylet::mptIssuance(sleVault->at(sfShareMPTID))); + BEAST_EXPECT(sleIssuance != nullptr); + + return sleIssuance->at(sfOutstandingAmount); + }; + + auto const setupVault = + [&](PrettyAsset const& asset, + Account const& owner, + Account const& depositor) -> std::pair { + Vault vault{env}; + + auto const& [tx, vaultKeylet] = + vault.create({.owner = owner, .asset = asset}); + env(tx, ter(tesSUCCESS), THISLINE); + env.close(); + + auto const& vaultSle = env.le(vaultKeylet); + BEAST_EXPECT(vaultSle != nullptr); + + Asset share = vaultSle->at(sfShareMPTID); + + env(vault.deposit( + {.depositor = depositor, + .id = vaultKeylet.key, + .amount = asset(100)}), + ter(tesSUCCESS), + THISLINE); + env.close(); + + auto const& [availablePreDefault, totalPreDefault] = + vaultAssetBalance(vaultKeylet); + BEAST_EXPECT(availablePreDefault == totalPreDefault); + BEAST_EXPECT(availablePreDefault == asset(100).value()); + + // attempt to clawback shares while there are assets fails + env(vault.clawback( + {.issuer = owner, + .id = vaultKeylet.key, + .holder = depositor, + .amount = share(0).value()}), + ter(tecNO_PERMISSION), + THISLINE); + env.close(); + + auto const& sharesAvailable = vaultShareBalance(vaultKeylet); + auto const& brokerKeylet = + keylet::loanbroker(owner.id(), env.seq(owner)); + + env(set(owner, vaultKeylet.key), THISLINE); + env.close(); + + auto const& loanKeylet = keylet::loan(brokerKeylet.key, 1); + + // Create a simple Loan for the full amount of Vault assets + env(set(depositor, brokerKeylet.key, asset(100).value()), + loan::interestRate(TenthBips32(0)), + gracePeriod(10), + paymentInterval(120), + paymentTotal(10), + sig(sfCounterpartySignature, owner), + fee(env.current()->fees().base * 2), + ter(tesSUCCESS), + THISLINE); + env.close(); + + // attempt to clawback shares while there assetsAvailable == 0 and + // assetsTotal > 0 fails + env(vault.clawback( + {.issuer = owner, + .id = vaultKeylet.key, + .holder = depositor, + .amount = share(0).value()}), + ter(tecNO_PERMISSION), + THISLINE); + env.close(); + + env.close(std::chrono::seconds{120 + 10}); + + env(manage(owner, loanKeylet.key, tfLoanDefault), + ter(tesSUCCESS), + THISLINE); + + auto const& [availablePostDefault, totalPostDefault] = + vaultAssetBalance(vaultKeylet); + + BEAST_EXPECT(availablePostDefault == totalPostDefault); + BEAST_EXPECT(availablePostDefault == asset(0).value()); + BEAST_EXPECT(vaultShareBalance(vaultKeylet) == sharesAvailable); + + return std::make_pair(vault, vaultKeylet); + }; + + auto const testCase = [&](PrettyAsset const& asset, + std::string const& prefix, + Account const& owner, + Account const& depositor) { + { + testcase( + "VaultClawback (share) - " + prefix + + " owner asset clawback fails"); + auto [vault, vaultKeylet] = setupVault(asset, owner, depositor); + env(vault.clawback({ + .issuer = owner, + .id = vaultKeylet.key, + .holder = depositor, + .amount = asset(100).value(), + }), + // when asset is XRP or owner is not issuer clawback fail + // when owner is issuer precision loss occurs as vault is + // empty + asset.native() ? ter(temMALFORMED) + : asset.raw().getIssuer() != owner.id() + ? ter(tecNO_PERMISSION) + : ter(tecPRECISION_LOSS), + THISLINE); + env.close(); + } + + { + testcase( + "VaultClawback (share) - " + prefix + + " owner incomplete share clawback fails"); + auto [vault, vaultKeylet] = setupVault(asset, owner, depositor); + auto const& vaultSle = env.le(vaultKeylet); + BEAST_EXPECT(vaultSle != nullptr); + if (!vaultSle) + return; + Asset share = vaultSle->at(sfShareMPTID); + env(vault.clawback({ + .issuer = owner, + .id = vaultKeylet.key, + .holder = depositor, + .amount = share(1).value(), + }), + ter(tecLIMIT_EXCEEDED), + THISLINE); + env.close(); + } + + { + testcase( + "VaultClawback (share) - " + prefix + + " owner implicit complete share clawback"); + auto [vault, vaultKeylet] = setupVault(asset, owner, depositor); + env(vault.clawback({ + .issuer = owner, + .id = vaultKeylet.key, + .holder = depositor, + }), + // when owner is issuer implicit clawback fails + asset.native() || asset.raw().getIssuer() != owner.id() + ? ter(tesSUCCESS) + : ter(tecWRONG_ASSET), + THISLINE); + env.close(); + } + + { + testcase( + "VaultClawback (share) - " + prefix + + " owner explicit complete share clawback succeeds"); + auto [vault, vaultKeylet] = setupVault(asset, owner, depositor); + auto const& vaultSle = env.le(vaultKeylet); + BEAST_EXPECT(vaultSle != nullptr); + if (!vaultSle) + return; + Asset share = vaultSle->at(sfShareMPTID); + env(vault.clawback({ + .issuer = owner, + .id = vaultKeylet.key, + .holder = depositor, + .amount = share(vaultShareBalance(vaultKeylet)).value(), + }), + ter(tesSUCCESS), + THISLINE); + env.close(); + } + { + testcase( + "VaultClawback (share) - " + prefix + + " owner can clawback own shares"); + auto [vault, vaultKeylet] = setupVault(asset, owner, owner); + auto const& vaultSle = env.le(vaultKeylet); + BEAST_EXPECT(vaultSle != nullptr); + if (!vaultSle) + return; + Asset share = vaultSle->at(sfShareMPTID); + env(vault.clawback({ + .issuer = owner, + .id = vaultKeylet.key, + .holder = owner, + .amount = share(vaultShareBalance(vaultKeylet)).value(), + }), + ter(tesSUCCESS), + THISLINE); + env.close(); + } + + { + testcase( + "VaultClawback (share) - " + prefix + + " empty vault share clawback fails"); + auto [vault, vaultKeylet] = setupVault(asset, owner, owner); + auto const& vaultSle = env.le(vaultKeylet); + if (BEAST_EXPECT(vaultSle != nullptr)) + return; + Asset share = vaultSle->at(sfShareMPTID); + env(vault.clawback({ + .issuer = owner, + .id = vaultKeylet.key, + .holder = owner, + .amount = share(vaultShareBalance(vaultKeylet)).value(), + }), + ter(tesSUCCESS), + THISLINE); + + // Now the vault is empty, clawback again fails + env(vault.clawback({ + .issuer = owner, + .id = vaultKeylet.key, + .holder = owner, + }), + ter(tecNO_PERMISSION), + THISLINE); + env.close(); + } + }; + + Account owner{"alice"}; + Account depositor{"bob"}; + Account issuer{"issuer"}; + + env.fund(XRP(10000), issuer, owner, depositor); + env.close(); + + // Test XRP + PrettyAsset xrp = xrpIssue(); + testCase(xrp, "XRP", owner, depositor); + testCase(xrp, "XRP (depositor is owner)", owner, owner); + + // Test IOU + PrettyAsset IOU = issuer["IOU"]; + env(fset(issuer, asfAllowTrustLineClawback)); + env.close(); + + env.trust(IOU(1000), owner); + env.trust(IOU(1000), depositor); + env(pay(issuer, owner, IOU(100))); + env(pay(issuer, depositor, IOU(100))); + env.close(); + testCase(IOU, "IOU", owner, depositor); + testCase(IOU, "IOU (owner is issuer)", issuer, depositor); + + // Test MPT + MPTTester mptt{env, issuer, mptInitNoFund}; + mptt.create( + {.flags = tfMPTCanClawback | tfMPTCanTransfer | tfMPTCanLock}); + PrettyAsset MPT = mptt.issuanceID(); + mptt.authorize({.account = owner}); + mptt.authorize({.account = depositor}); + env(pay(issuer, owner, MPT(1000))); + env(pay(issuer, depositor, MPT(1000))); + env.close(); + testCase(MPT, "MPT", owner, depositor); + testCase(MPT, "MPT (owner is issuer)", issuer, depositor); + } + + void + testVaultClawbackAssets() + { + using namespace test::jtx; + using namespace loanBroker; + using namespace loan; + Env env(*this); + + auto const setupVault = + [&](PrettyAsset const& asset, + Account const& owner, + Account const& depositor, + Account const& issuer) -> std::pair { + Vault vault{env}; + + auto const& [tx, vaultKeylet] = + vault.create({.owner = owner, .asset = asset}); + env(tx, ter(tesSUCCESS), THISLINE); + env.close(); + + auto const& vaultSle = env.le(vaultKeylet); + BEAST_EXPECT(vaultSle != nullptr); + env(vault.deposit( + {.depositor = depositor, + .id = vaultKeylet.key, + .amount = asset(100)}), + ter(tesSUCCESS), + THISLINE); + env.close(); + + return std::make_pair(vault, vaultKeylet); + }; + + auto const testCase = [&](PrettyAsset const& asset, + std::string const& prefix, + Account const& owner, + Account const& depositor, + Account const& issuer) { + if (asset.native()) + { + testcase( + "VaultClawback (asset) - " + prefix + + " issuer XRP clawback fails"); + auto [vault, vaultKeylet] = + setupVault(asset, owner, depositor, issuer); + // If the asset is XRP, clawback with amount fails as malfored + // when asset is specified. + env(vault.clawback({ + .issuer = issuer, + .id = vaultKeylet.key, + .holder = issuer, + .amount = asset(1).value(), + }), + ter(temMALFORMED), + THISLINE); + // When asset is implicit, clawback fails as no permission. + env(vault.clawback({ + .issuer = issuer, + .id = vaultKeylet.key, + .holder = issuer, + }), + ter(tecNO_PERMISSION), + THISLINE); + return; + } + + { + testcase( + "VaultClawback (asset) - " + prefix + + " clawback for different asset fails"); + auto [vault, vaultKeylet] = + setupVault(asset, owner, depositor, issuer); + + Account issuer2{"issuer2"}; + PrettyAsset asset2 = issuer2["FOO"]; + env(vault.clawback({ + .issuer = issuer, + .id = vaultKeylet.key, + .holder = depositor, + .amount = asset2(1).value(), + }), + ter(tecWRONG_ASSET), + THISLINE); + } + + { + testcase( + "VaultClawback (asset) - " + prefix + + " ambiguous owner/issuer asset clawback fails"); + auto [vault, vaultKeylet] = + setupVault(asset, issuer, depositor, issuer); + env(vault.clawback({ + .issuer = issuer, + .id = vaultKeylet.key, + .holder = issuer, + }), + ter(tecWRONG_ASSET), + THISLINE); + } + + { + testcase( + "VaultClawback (asset) - " + prefix + + " non-issuer asset clawback fails"); + auto [vault, vaultKeylet] = + setupVault(asset, owner, depositor, issuer); + + env(vault.clawback({ + .issuer = owner, + .id = vaultKeylet.key, + .holder = depositor, + }), + ter(tecNO_PERMISSION), + THISLINE); + + env(vault.clawback({ + .issuer = owner, + .id = vaultKeylet.key, + .holder = depositor, + .amount = asset(1).value(), + }), + ter(tecNO_PERMISSION), + THISLINE); + } + + { + testcase( + "VaultClawback (asset) - " + prefix + + " issuer clawback from self fails"); + auto [vault, vaultKeylet] = + setupVault(asset, owner, issuer, issuer); + env(vault.clawback({ + .issuer = issuer, + .id = vaultKeylet.key, + .holder = issuer, + }), + ter(tecNO_PERMISSION), + THISLINE); + } + + { + testcase( + "VaultClawback (asset) - " + prefix + + " issuer share clawback fails"); + auto [vault, vaultKeylet] = + setupVault(asset, owner, depositor, issuer); + auto const& vaultSle = env.le(vaultKeylet); + BEAST_EXPECT(vaultSle != nullptr); + if (!vaultSle) + return; + Asset share = vaultSle->at(sfShareMPTID); + + env(vault.clawback({ + .issuer = issuer, + .id = vaultKeylet.key, + .holder = depositor, + .amount = share(1).value(), + }), + ter(tecNO_PERMISSION), + THISLINE); + } + + { + testcase( + "VaultClawback (asset) - " + prefix + + " partial issuer asset clawback succeeds"); + auto [vault, vaultKeylet] = + setupVault(asset, owner, depositor, issuer); + + env(vault.clawback({ + .issuer = issuer, + .id = vaultKeylet.key, + .holder = depositor, + .amount = asset(1).value(), + }), + ter(tesSUCCESS), + THISLINE); + } + + { + testcase( + "VaultClawback (asset) - " + prefix + + " full issuer asset clawback succeeds"); + auto [vault, vaultKeylet] = + setupVault(asset, owner, depositor, issuer); + + env(vault.clawback({ + .issuer = issuer, + .id = vaultKeylet.key, + .holder = depositor, + .amount = asset(100).value(), + }), + ter(tesSUCCESS), + THISLINE); + } + + { + testcase( + "VaultClawback (asset) - " + prefix + + " implicit full issuer asset clawback succeeds"); + auto [vault, vaultKeylet] = + setupVault(asset, owner, depositor, issuer); + + env(vault.clawback({ + .issuer = issuer, + .id = vaultKeylet.key, + .holder = depositor, + }), + ter(tesSUCCESS), + THISLINE); + } + }; + + Account owner{"alice"}; + Account depositor{"bob"}; + Account issuer{"issuer"}; + + env.fund(XRP(10000), issuer, owner, depositor); + env.close(); + + // Test XRP + PrettyAsset xrp = xrpIssue(); + testCase(xrp, "XRP", owner, depositor, issuer); + + // Test IOU + PrettyAsset IOU = issuer["IOU"]; + env(fset(issuer, asfAllowTrustLineClawback)); + env.close(); + env.trust(IOU(1000), owner); + env.trust(IOU(1000), depositor); + env(pay(issuer, owner, IOU(1000))); + env(pay(issuer, depositor, IOU(1000))); + env.close(); + testCase(IOU, "IOU", owner, depositor, issuer); + + // Test MPT + MPTTester mptt{env, issuer, mptInitNoFund}; + mptt.create( + {.flags = tfMPTCanClawback | tfMPTCanTransfer | tfMPTCanLock}); + PrettyAsset MPT = mptt.issuanceID(); + mptt.authorize({.account = owner}); + mptt.authorize({.account = depositor}); + env(pay(issuer, depositor, MPT(1000))); + env.close(); + testCase(MPT, "MPT", owner, depositor, issuer); + } + public: void run() override @@ -5261,6 +5800,8 @@ public: testScaleIOU(); testRPC(); testDelegate(); + testVaultClawbackBurnShares(); + testVaultClawbackAssets(); } }; diff --git a/src/xrpld/app/tx/detail/InvariantCheck.cpp b/src/xrpld/app/tx/detail/InvariantCheck.cpp index 0b237905e8..2e0b3cbfab 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.cpp +++ b/src/xrpld/app/tx/detail/InvariantCheck.cpp @@ -95,6 +95,7 @@ hasPrivilege(STTx const& tx, Privilege priv) switch (tx.getTxnType()) { #include + // Deprecated types default: return false; @@ -2622,6 +2623,7 @@ ValidVault::Vault::make(SLE const& from) self.key = from.key(); self.asset = from.at(sfAsset); self.pseudoId = from.getAccountID(sfAccount); + self.owner = from.at(sfOwner); self.shareMPTID = from.getFieldH192(sfShareMPTID); self.assetsTotal = from.at(sfAssetsTotal); self.assetsAvailable = from.at(sfAssetsAvailable); @@ -3066,6 +3068,10 @@ ValidVault::finalize( : std::nullopt; }; + auto const vaultHoldsNoAssets = [&](Vault const& vault) { + return vault.assetsAvailable == 0 && vault.assetsTotal == 0; + }; + // Technically this does not need to be a lambda, but it's more // convenient thanks to early "return false"; the not-so-nice // alternatives are several layers of nested if/else or more complex @@ -3448,29 +3454,56 @@ ValidVault::finalize( if (vaultAsset.native() || vaultAsset.getIssuer() != tx[sfAccount]) { - JLOG(j.fatal()) << // - "Invariant failed: clawback may only be performed by " - "the asset issuer"; - return false; // That's all we can do + // The owner can use clawback to force-burn shares when the + // vault is empty but there are outstanding shares + if (!(beforeShares && beforeShares->sharesTotal > 0 && + vaultHoldsNoAssets(beforeVault) && + beforeVault.owner == tx[sfAccount])) + { + JLOG(j.fatal()) << // + "Invariant failed: clawback may only be performed " + "by the asset issuer, or by the vault owner of an " + "empty vault"; + return false; // That's all we can do + } } auto const vaultDeltaAssets = deltaAssets(afterVault.pseudoId); + if (vaultDeltaAssets) + { + if (*vaultDeltaAssets >= zero) + { + JLOG(j.fatal()) << // + "Invariant failed: clawback must decrease vault " + "balance"; + result = false; + } - if (!vaultDeltaAssets) + if (beforeVault.assetsTotal + *vaultDeltaAssets != + afterVault.assetsTotal) + { + JLOG(j.fatal()) << // + "Invariant failed: clawback and assets outstanding " + "must add up"; + result = false; + } + + if (beforeVault.assetsAvailable + *vaultDeltaAssets != + afterVault.assetsAvailable) + { + JLOG(j.fatal()) << // + "Invariant failed: clawback and assets available " + "must add up"; + result = false; + } + } + else if (!vaultHoldsNoAssets(beforeVault)) { JLOG(j.fatal()) << // "Invariant failed: clawback must change vault balance"; return false; // That's all we can do } - if (*vaultDeltaAssets >= zero) - { - JLOG(j.fatal()) << // - "Invariant failed: clawback must decrease vault " - "balance"; - result = false; - } - auto const accountDeltaShares = deltaShares(tx[sfHolder]); if (!accountDeltaShares) { @@ -3503,24 +3536,6 @@ ValidVault::finalize( result = false; } - if (beforeVault.assetsTotal + *vaultDeltaAssets != - afterVault.assetsTotal) - { - JLOG(j.fatal()) << // - "Invariant failed: clawback and assets outstanding " - "must add up"; - result = false; - } - - if (beforeVault.assetsAvailable + *vaultDeltaAssets != - afterVault.assetsAvailable) - { - JLOG(j.fatal()) << // - "Invariant failed: clawback and assets available must " - "add up"; - result = false; - } - return result; } diff --git a/src/xrpld/app/tx/detail/InvariantCheck.h b/src/xrpld/app/tx/detail/InvariantCheck.h index ef9db373f5..87a1afb623 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.h +++ b/src/xrpld/app/tx/detail/InvariantCheck.h @@ -861,6 +861,7 @@ class ValidVault uint256 key = beast::zero; Asset asset = {}; AccountID pseudoId = {}; + AccountID owner = {}; uint192 shareMPTID = beast::zero; Number assetsTotal = 0; Number assetsAvailable = 0; diff --git a/src/xrpld/app/tx/detail/VaultClawback.cpp b/src/xrpld/app/tx/detail/VaultClawback.cpp index cc7dec993a..2552e8c1ff 100644 --- a/src/xrpld/app/tx/detail/VaultClawback.cpp +++ b/src/xrpld/app/tx/detail/VaultClawback.cpp @@ -1,18 +1,17 @@ #include - +// #include #include #include -#include #include #include #include #include #include -#include + +#include namespace xrpl { - NotTEC VaultClawback::preflight(PreflightContext const& ctx) { @@ -22,15 +21,6 @@ VaultClawback::preflight(PreflightContext const& ctx) return temMALFORMED; } - AccountID const issuer = ctx.tx[sfAccount]; - AccountID const holder = ctx.tx[sfHolder]; - - if (issuer == holder) - { - JLOG(ctx.j.debug()) << "VaultClawback: issuer cannot be holder."; - return temMALFORMED; - } - auto const amount = ctx.tx[~sfAmount]; if (amount) { @@ -42,17 +32,27 @@ VaultClawback::preflight(PreflightContext const& ctx) JLOG(ctx.j.debug()) << "VaultClawback: cannot clawback XRP."; return temMALFORMED; } - else if (amount->asset().getIssuer() != issuer) - { - JLOG(ctx.j.debug()) - << "VaultClawback: only asset issuer can clawback."; - return temMALFORMED; - } } return tesSUCCESS; } +[[nodiscard]] STAmount +clawbackAmount( + std::shared_ptr const& vault, + std::optional const& maybeAmount, + AccountID const& account) +{ + if (maybeAmount) + return *maybeAmount; + + Asset const share = MPTIssue{vault->at(sfShareMPTID)}; + if (account == vault->at(sfOwner)) + return STAmount{share}; + + return STAmount{vault->at(sfAsset)}; +} + TER VaultClawback::preclaim(PreclaimContext const& ctx) { @@ -60,61 +60,264 @@ VaultClawback::preclaim(PreclaimContext const& ctx) if (!vault) return tecNO_ENTRY; - auto account = ctx.tx[sfAccount]; - auto const issuer = ctx.view.read(keylet::account(account)); - if (!issuer) + Asset const vaultAsset = vault->at(sfAsset); + auto const account = ctx.tx[sfAccount]; + auto const holder = ctx.tx[sfHolder]; + auto const maybeAmount = ctx.tx[~sfAmount]; + auto const mptIssuanceID = vault->at(sfShareMPTID); + auto const sleShareIssuance = + ctx.view.read(keylet::mptIssuance(mptIssuanceID)); + if (!sleShareIssuance) { // LCOV_EXCL_START - JLOG(ctx.j.error()) << "VaultClawback: missing issuer account."; + JLOG(ctx.j.error()) + << "VaultClawback: missing issuance of vault shares."; return tefINTERNAL; // LCOV_EXCL_STOP } - Asset const vaultAsset = vault->at(sfAsset); - if (auto const amount = ctx.tx[~sfAmount]; - amount && vaultAsset != amount->asset()) + Asset const share = MPTIssue{mptIssuanceID}; + + // Ambiguous case: If Issuer is Owner they must specify the asset + if (!maybeAmount && !vaultAsset.native() && + vaultAsset.getIssuer() == vault->at(sfOwner)) + { + JLOG(ctx.j.debug()) + << "VaultClawback: must specify amount when issuer is owner."; return tecWRONG_ASSET; - - if (vaultAsset.native()) - { - JLOG(ctx.j.debug()) << "VaultClawback: cannot clawback XRP."; - return tecNO_PERMISSION; // Cannot clawback XRP. - } - else if (vaultAsset.getIssuer() != account) - { - JLOG(ctx.j.debug()) << "VaultClawback: only asset issuer can clawback."; - return tecNO_PERMISSION; // Only issuers can clawback. } - if (vaultAsset.holds()) - { - auto const mpt = vaultAsset.get(); - auto const mptIssue = - ctx.view.read(keylet::mptIssuance(mpt.getMptID())); - if (mptIssue == nullptr) - return tecOBJECT_NOT_FOUND; + auto const amount = clawbackAmount(vault, maybeAmount, account); - std::uint32_t const issueFlags = mptIssue->getFieldU32(sfFlags); - if (!(issueFlags & lsfMPTCanClawback)) + // There is a special case that allows the VaultOwner to use clawback to + // burn shares when Vault assets total and available are zero, but + // shares remain. However, that case is handled in doApply() directly, + // so here we just enforce checks. + if (amount.asset() == share) + { + // Only the Vault Owner may clawback shares + if (account != vault->at(sfOwner)) { JLOG(ctx.j.debug()) - << "VaultClawback: cannot clawback MPT vault asset."; + << "VaultClawback: only vault owner can clawback shares."; return tecNO_PERMISSION; } - } - else if (vaultAsset.holds()) - { - std::uint32_t const issuerFlags = issuer->getFieldU32(sfFlags); - if (!(issuerFlags & lsfAllowTrustLineClawback) || - (issuerFlags & lsfNoFreeze)) + + auto const assetsTotal = vault->at(sfAssetsTotal); + auto const assetsAvailable = vault->at(sfAssetsAvailable); + auto const sharesTotal = sleShareIssuance->at(sfOutstandingAmount); + + // Owner can clawback funds when the vault has shares but no assets + if (sharesTotal == 0 || (assetsTotal != 0 || assetsAvailable != 0)) { JLOG(ctx.j.debug()) - << "VaultClawback: cannot clawback IOU vault asset."; + << "VaultClawback: vault owner can clawback shares only" + " when vault has no assets."; return tecNO_PERMISSION; } + + // If amount is non-zero, the VaultOwner must burn all shares + if (amount != beast::zero) + { + Number const& sharesHeld = accountHolds( + ctx.view, + holder, + share, + FreezeHandling::fhIGNORE_FREEZE, + AuthHandling::ahIGNORE_AUTH, + ctx.j); + + // The VaultOwner must burn all shares + if (amount != sharesHeld) + { + JLOG(ctx.j.debug()) + << "VaultClawback: vault owner must clawback all " + "shares."; + return tecLIMIT_EXCEEDED; + } + } + + return tesSUCCESS; } - return tesSUCCESS; + // The asset that is being clawed back is the vault asset + if (amount.asset() == vaultAsset) + { + // XRP cannot be clawed back + if (vaultAsset.native()) + { + JLOG(ctx.j.debug()) << "VaultClawback: cannot clawback XRP."; + return tecNO_PERMISSION; + } + + // Only the Asset Issuer may clawback the asset + if (account != vaultAsset.getIssuer()) + { + JLOG(ctx.j.debug()) + << "VaultClawback: only asset issuer can clawback asset."; + return tecNO_PERMISSION; + } + + // The issuer cannot clawback from itself + if (account == holder) + { + JLOG(ctx.j.debug()) + << "VaultClawback: issuer cannot be the holder."; + return tecNO_PERMISSION; + } + + return std::visit( + [&](TIss const& issue) -> TER { + if constexpr (std::is_same_v) + { + auto const mptIssue = + ctx.view.read(keylet::mptIssuance(issue.getMptID())); + if (mptIssue == nullptr) + return tecOBJECT_NOT_FOUND; + + std::uint32_t const issueFlags = + mptIssue->getFieldU32(sfFlags); + if (!(issueFlags & lsfMPTCanClawback)) + { + JLOG(ctx.j.debug()) << "VaultClawback: cannot clawback " + "MPT vault asset."; + return tecNO_PERMISSION; + } + } + else if constexpr (std::is_same_v) + { + auto const issuerSle = + ctx.view.read(keylet::account(account)); + if (!issuerSle) + { + // LCOV_EXCL_START + JLOG(ctx.j.error()) + << "VaultClawback: missing submitter account."; + return tefINTERNAL; + // LCOV_EXCL_STOP + } + + std::uint32_t const issuerFlags = + issuerSle->getFieldU32(sfFlags); + if (!(issuerFlags & lsfAllowTrustLineClawback) || + (issuerFlags & lsfNoFreeze)) + { + JLOG(ctx.j.debug()) << "VaultClawback: cannot clawback " + "IOU vault asset."; + return tecNO_PERMISSION; + } + } + return tesSUCCESS; + }, + vaultAsset.value()); + } + + // Invalid asset + return tecWRONG_ASSET; +} + +Expected, TER> +VaultClawback::assetsToClawback( + std::shared_ptr const& vault, + std::shared_ptr const& sleShareIssuance, + AccountID const& holder, + STAmount const& clawbackAmount) +{ + if (clawbackAmount.asset() != vault->at(sfAsset)) + { + // preclaim should have blocked this , now it's an internal error + // LCOV_EXCL_START + JLOG(j_.error()) << "VaultClawback: asset mismatch in clawback."; + return Unexpected(tecINTERNAL); + // LCOV_EXCL_STOP + } + + auto const assetsAvailable = vault->at(sfAssetsAvailable); + auto const mptIssuanceID = *vault->at(sfShareMPTID); + MPTIssue const share{mptIssuanceID}; + + if (clawbackAmount == beast::zero) + { + auto const sharesDestroyed = accountHolds( + view(), + holder, + share, + FreezeHandling::fhIGNORE_FREEZE, + AuthHandling::ahIGNORE_AUTH, + j_); + auto const maybeAssets = + sharesToAssetsWithdraw(vault, sleShareIssuance, sharesDestroyed); + if (!maybeAssets) + return Unexpected(tecINTERNAL); // LCOV_EXCL_LINE + + return std::make_pair(*maybeAssets, sharesDestroyed); + } + + STAmount sharesDestroyed; + STAmount assetsRecovered = clawbackAmount; + try + { + { + auto const maybeShares = assetsToSharesWithdraw( + vault, sleShareIssuance, assetsRecovered); + if (!maybeShares) + return Unexpected(tecINTERNAL); // LCOV_EXCL_LINE + sharesDestroyed = *maybeShares; + } + + auto const maybeAssets = + sharesToAssetsWithdraw(vault, sleShareIssuance, sharesDestroyed); + if (!maybeAssets) + return Unexpected(tecINTERNAL); // LCOV_EXCL_LINE + assetsRecovered = *maybeAssets; + + // Clamp to maximum. + if (assetsRecovered > *assetsAvailable) + { + assetsRecovered = *assetsAvailable; + // Note, it is important to truncate the number of shares, + // otherwise the corresponding assets might breach the + // AssetsAvailable + { + auto const maybeShares = assetsToSharesWithdraw( + vault, + sleShareIssuance, + assetsRecovered, + TruncateShares::yes); + if (!maybeShares) + return Unexpected(tecINTERNAL); // LCOV_EXCL_LINE + sharesDestroyed = *maybeShares; + } + + auto const maybeAssets = sharesToAssetsWithdraw( + vault, sleShareIssuance, sharesDestroyed); + if (!maybeAssets) + return Unexpected(tecINTERNAL); // LCOV_EXCL_LINE + assetsRecovered = *maybeAssets; + if (assetsRecovered > *assetsAvailable) + { + // LCOV_EXCL_START + JLOG(j_.error()) + << "VaultClawback: invalid rounding of shares."; + return Unexpected(tecINTERNAL); + // LCOV_EXCL_STOP + } + } + } + catch (std::overflow_error const&) + { + // It's easy to hit this exception from Number with large enough + // Scale so we avoid spamming the log and only use debug here. + JLOG(j_.debug()) // + << "VaultClawback: overflow error with" + << " scale=" << (int)vault->at(sfScale).value() // + << ", assetsTotal=" << vault->at(sfAssetsTotal).value() + << ", sharesTotal=" << sleShareIssuance->at(sfOutstandingAmount) + << ", amount=" << clawbackAmount.value(); + return Unexpected(tecPATH_DRY); + } + + return std::make_pair(assetsRecovered, sharesDestroyed); } TER @@ -125,7 +328,7 @@ VaultClawback::doApply() if (!vault) return tefINTERNAL; // LCOV_EXCL_LINE - auto const mptIssuanceID = *((*vault)[sfShareMPTID]); + auto const mptIssuanceID = *vault->at(sfShareMPTID); auto const sleIssuance = view().read(keylet::mptIssuance(mptIssuanceID)); if (!sleIssuance) { @@ -134,105 +337,47 @@ VaultClawback::doApply() return tefINTERNAL; // LCOV_EXCL_STOP } + MPTIssue const share{mptIssuanceID}; Asset const vaultAsset = vault->at(sfAsset); - STAmount const amount = [&]() -> STAmount { - auto const maybeAmount = tx[~sfAmount]; - if (maybeAmount) - return *maybeAmount; - return {sfAmount, vaultAsset, 0}; - }(); - XRPL_ASSERT( - amount.asset() == vaultAsset, - "xrpl::VaultClawback::doApply : matching asset"); + STAmount const amount = clawbackAmount(vault, tx[~sfAmount], account_); auto assetsAvailable = vault->at(sfAssetsAvailable); auto assetsTotal = vault->at(sfAssetsTotal); + [[maybe_unused]] auto const lossUnrealized = vault->at(sfLossUnrealized); XRPL_ASSERT( lossUnrealized <= (assetsTotal - assetsAvailable), "xrpl::VaultClawback::doApply : loss and assets do balance"); AccountID holder = tx[sfHolder]; - MPTIssue const share{mptIssuanceID}; STAmount sharesDestroyed = {share}; - STAmount assetsRecovered; - try + STAmount assetsRecovered = {vault->at(sfAsset)}; + + // The Owner is burning shares + if (account_ == vault->at(sfOwner) && amount.asset() == share) { - if (amount == beast::zero) - { - sharesDestroyed = accountHolds( - view(), - holder, - share, - FreezeHandling::fhIGNORE_FREEZE, - AuthHandling::ahIGNORE_AUTH, - j_); - - auto const maybeAssets = - sharesToAssetsWithdraw(vault, sleIssuance, sharesDestroyed); - if (!maybeAssets) - return tecINTERNAL; // LCOV_EXCL_LINE - assetsRecovered = *maybeAssets; - } - else - { - assetsRecovered = amount; - { - auto const maybeShares = - assetsToSharesWithdraw(vault, sleIssuance, assetsRecovered); - if (!maybeShares) - return tecINTERNAL; // LCOV_EXCL_LINE - sharesDestroyed = *maybeShares; - } - - auto const maybeAssets = - sharesToAssetsWithdraw(vault, sleIssuance, sharesDestroyed); - if (!maybeAssets) - return tecINTERNAL; // LCOV_EXCL_LINE - assetsRecovered = *maybeAssets; - } - - // Clamp to maximum. - if (assetsRecovered > *assetsAvailable) - { - assetsRecovered = *assetsAvailable; - // Note, it is important to truncate the number of shares, otherwise - // the corresponding assets might breach the AssetsAvailable - { - auto const maybeShares = assetsToSharesWithdraw( - vault, sleIssuance, assetsRecovered, TruncateShares::yes); - if (!maybeShares) - return tecINTERNAL; // LCOV_EXCL_LINE - sharesDestroyed = *maybeShares; - } - - auto const maybeAssets = - sharesToAssetsWithdraw(vault, sleIssuance, sharesDestroyed); - if (!maybeAssets) - return tecINTERNAL; // LCOV_EXCL_LINE - assetsRecovered = *maybeAssets; - if (assetsRecovered > *assetsAvailable) - { - // LCOV_EXCL_START - JLOG(j_.error()) - << "VaultClawback: invalid rounding of shares."; - return tecINTERNAL; - // LCOV_EXCL_STOP - } - } + sharesDestroyed = accountHolds( + view(), + holder, + share, + FreezeHandling::fhIGNORE_FREEZE, + AuthHandling::ahIGNORE_AUTH, + j_); } - catch (std::overflow_error const&) + else // The Issuer is clawbacking vault assets { - // It's easy to hit this exception from Number with large enough Scale - // so we avoid spamming the log and only use debug here. - JLOG(j_.debug()) // - << "VaultClawback: overflow error with" - << " scale=" << (int)vault->at(sfScale).value() // - << ", assetsTotal=" << vault->at(sfAssetsTotal).value() - << ", sharesTotal=" << sleIssuance->at(sfOutstandingAmount) - << ", amount=" << amount.value(); - return tecPATH_DRY; + XRPL_ASSERT( + amount.asset() == vaultAsset, + "xrpl::VaultClawback::doApply : matching asset"); + + auto const clawbackParts = + assetsToClawback(vault, sleIssuance, holder, amount); + if (!clawbackParts) + return clawbackParts.error(); + + assetsRecovered = clawbackParts->first; + sharesDestroyed = clawbackParts->second; } if (sharesDestroyed == beast::zero) @@ -282,30 +427,34 @@ VaultClawback::doApply() // else quietly ignore, holder balance is not zero } - // Transfer assets from vault to issuer. - if (auto const ter = accountSend( - view(), - vaultAccount, - account_, - assetsRecovered, - j_, - WaiveTransferFee::Yes); - !isTesSuccess(ter)) - return ter; - - // Sanity check - if (accountHolds( - view(), - vaultAccount, - assetsRecovered.asset(), - FreezeHandling::fhIGNORE_FREEZE, - AuthHandling::ahIGNORE_AUTH, - j_) < beast::zero) + if (assetsRecovered > beast::zero) { - // LCOV_EXCL_START - JLOG(j_.error()) << "VaultClawback: negative balance of vault assets."; - return tefINTERNAL; - // LCOV_EXCL_STOP + // Transfer assets from vault to issuer. + if (auto const ter = accountSend( + view(), + vaultAccount, + account_, + assetsRecovered, + j_, + WaiveTransferFee::Yes); + !isTesSuccess(ter)) + return ter; + + // Sanity check + if (accountHolds( + view(), + vaultAccount, + assetsRecovered.asset(), + FreezeHandling::fhIGNORE_FREEZE, + AuthHandling::ahIGNORE_AUTH, + j_) < beast::zero) + { + // LCOV_EXCL_START + JLOG(j_.error()) + << "VaultClawback: negative balance of vault assets."; + return tefINTERNAL; + // LCOV_EXCL_STOP + } } return tesSUCCESS; diff --git a/src/xrpld/app/tx/detail/VaultClawback.h b/src/xrpld/app/tx/detail/VaultClawback.h index 80a5f73ad0..d05f280e75 100644 --- a/src/xrpld/app/tx/detail/VaultClawback.h +++ b/src/xrpld/app/tx/detail/VaultClawback.h @@ -22,6 +22,14 @@ public: TER doApply() override; + +private: + Expected, TER> + assetsToClawback( + std::shared_ptr const& vault, + std::shared_ptr const& sleShareIssuance, + AccountID const& holder, + STAmount const& clawbackAmount); }; } // namespace xrpl From 7c1183547abb054878b24dd06fc2aab4e01ca350 Mon Sep 17 00:00:00 2001 From: Bart Date: Fri, 9 Jan 2026 16:44:43 -0500 Subject: [PATCH 09/11] chore: Change `/Zi` to `/Z7` for ccache, remove debug symbols in CI (#6198) As the `/Zi` compiler flag is unsupported by ccache, this change switches it to `/Z7` instead. For CI runs all debug info is omitted. --- .config/cspell.config.yaml | 1 + .github/workflows/reusable-build-test-config.yml | 2 +- cmake/Ccache.cmake | 12 +++++++++--- cmake/XrplCompiler.cmake | 1 + 4 files changed, 12 insertions(+), 4 deletions(-) diff --git a/.config/cspell.config.yaml b/.config/cspell.config.yaml index 0cac82807d..edcdcc92ba 100644 --- a/.config/cspell.config.yaml +++ b/.config/cspell.config.yaml @@ -51,6 +51,7 @@ words: - Btrfs - canonicality - checkme + - choco - chrono - citardauq - clawback diff --git a/.github/workflows/reusable-build-test-config.yml b/.github/workflows/reusable-build-test-config.yml index fc80bbd216..ae91a8bf20 100644 --- a/.github/workflows/reusable-build-test-config.yml +++ b/.github/workflows/reusable-build-test-config.yml @@ -100,7 +100,7 @@ jobs: uses: actions/checkout@08eba0b27e820071cde6df949e0beb9ba4906955 # v4.3.0 - name: Prepare runner - uses: XRPLF/actions/prepare-runner@65da1c59e81965eeb257caa3587b9d45066fb925 + uses: XRPLF/actions/prepare-runner@121d1de2775d486d46140b9a91b32d5002c08153 with: enable_ccache: ${{ inputs.ccache_enabled }} diff --git a/cmake/Ccache.cmake b/cmake/Ccache.cmake index 092212075c..aa8d3ac59d 100644 --- a/cmake/Ccache.cmake +++ b/cmake/Ccache.cmake @@ -46,6 +46,12 @@ set(CMAKE_VS_GLOBALS "TrackFileAccess=false" "UseMultiToolTask=true") -# By default Visual Studio generators will use /Zi, which is not compatible with -# ccache, so tell it to use /Z7 instead. -set(CMAKE_MSVC_DEBUG_INFORMATION_FORMAT "$<$:Embedded>") +# By default Visual Studio generators will use /Zi to capture debug information, +# which is not compatible with ccache, so tell it to use /Z7 instead. +if (MSVC) + foreach (var_ + CMAKE_C_FLAGS_DEBUG CMAKE_C_FLAGS_RELEASE + CMAKE_CXX_FLAGS_DEBUG CMAKE_CXX_FLAGS_RELEASE) + string (REPLACE "/Zi" "/Z7" ${var_} "${${var_}}") + endforeach () +endif () diff --git a/cmake/XrplCompiler.cmake b/cmake/XrplCompiler.cmake index 622b2d2f74..0777bf948c 100644 --- a/cmake/XrplCompiler.cmake +++ b/cmake/XrplCompiler.cmake @@ -44,6 +44,7 @@ if (MSVC) # omit debug info completely under CI (not needed) if (is_ci) string (REPLACE "/Zi" " " ${var_} "${${var_}}") + string (REPLACE "/Z7" " " ${var_} "${${var_}}") endif () endforeach () From b2c5927b488fa0c306117de971f336c8d49317de Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 9 Jan 2026 23:10:04 -0400 Subject: [PATCH 10/11] fix: Inner batch transactions never have valid signatures (#6069) - Introduces amendment `fixBatchInnerSigs` - Update Batch unit tests - Fix all the Env instantiations to _use_ the "features" parameter. - testInnerSubmitRPC runs with Batch enabled and disabled. - Add a test to testInnerSubmitRPC for a correctly signed tx incorrectly using the tfInnerBatchTxn flag. - Generalize the submitAndValidate lambda in testInnerSubmitRPC. - With the fix amendment, a transaction never reaches the transaction engine (Transactor and derived classes.) - Test submitting a pseudo-transaction. Stopped before reaching the transaction engine, but with different errors. - The tests verify that without the amendment, a transaction with tfInnerBatchTxn is immediately rejected. Without the amendment, things are safe. The amendment just makes things safer and more future-proof. --- include/xrpl/protocol/detail/features.macro | 1 + src/test/app/Batch_test.cpp | 281 ++++++++++++++------ src/xrpld/app/misc/NetworkOPs.cpp | 2 +- src/xrpld/app/tx/detail/Transactor.cpp | 8 +- src/xrpld/app/tx/detail/apply.cpp | 21 +- 5 files changed, 221 insertions(+), 92 deletions(-) diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index 5c8d2aa198..932668c16f 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -16,6 +16,7 @@ // Add new amendments to the top of this list. // Keep it sorted in reverse chronological order. +XRPL_FIX (BatchInnerSigs, Supported::yes, VoteBehavior::DefaultNo) XRPL_FEATURE(LendingProtocol, Supported::no, VoteBehavior::DefaultNo) XRPL_FEATURE(PermissionDelegationV1_1, Supported::no, VoteBehavior::DefaultNo) XRPL_FIX (DirectoryLimit, Supported::yes, VoteBehavior::DefaultNo) diff --git a/src/test/app/Batch_test.cpp b/src/test/app/Batch_test.cpp index 6fbec52a93..68bf7e833b 100644 --- a/src/test/app/Batch_test.cpp +++ b/src/test/app/Batch_test.cpp @@ -148,15 +148,21 @@ class Batch_test : public beast::unit_test::suite void testEnable(FeatureBitset features) { - testcase("enabled"); - using namespace test::jtx; using namespace std::literals; + bool const withInnerSigFix = features[fixBatchInnerSigs]; + for (bool const withBatch : {true, false}) { + testcase << "enabled: Batch " + << (withBatch ? "enabled" : "disabled") + << ", Inner Sig Fix: " + << (withInnerSigFix ? "enabled" : "disabled"); + auto const amend = withBatch ? features : features - featureBatch; - test::jtx::Env env{*this, envconfig(), amend}; + + test::jtx::Env env{*this, amend}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -179,7 +185,7 @@ class Batch_test : public beast::unit_test::suite // tfInnerBatchTxn // If the feature is disabled, the transaction fails with - // temINVALID_FLAG If the feature is enabled, the transaction fails + // temINVALID_FLAG. If the feature is enabled, the transaction fails // early in checkValidity() { auto const txResult = @@ -205,7 +211,7 @@ class Batch_test : public beast::unit_test::suite //---------------------------------------------------------------------- // preflight - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -617,7 +623,7 @@ class Batch_test : public beast::unit_test::suite //---------------------------------------------------------------------- // preclaim - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -858,7 +864,7 @@ class Batch_test : public beast::unit_test::suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -949,7 +955,7 @@ class Batch_test : public beast::unit_test::suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -1187,7 +1193,7 @@ class Batch_test : public beast::unit_test::suite // Bad Fee Without Signer { - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -1209,7 +1215,7 @@ class Batch_test : public beast::unit_test::suite // Bad Fee With MultiSign { - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -1236,7 +1242,7 @@ class Batch_test : public beast::unit_test::suite // Bad Fee With MultiSign + BatchSigners { - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -1265,7 +1271,7 @@ class Batch_test : public beast::unit_test::suite // Bad Fee With MultiSign + BatchSigners.Signers { - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -1297,7 +1303,7 @@ class Batch_test : public beast::unit_test::suite // Bad Fee With BatchSigners { - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -1321,7 +1327,7 @@ class Batch_test : public beast::unit_test::suite // Bad Fee Dynamic Fee Calculation { - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -1361,7 +1367,7 @@ class Batch_test : public beast::unit_test::suite // telENV_RPC_FAILED: Batch: txns array exceeds 8 entries. { - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -1386,7 +1392,7 @@ class Batch_test : public beast::unit_test::suite // temARRAY_TOO_LARGE: Batch: txns array exceeds 8 entries. { - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -1419,7 +1425,7 @@ class Batch_test : public beast::unit_test::suite // telENV_RPC_FAILED: Batch: signers array exceeds 8 entries. { - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -1438,7 +1444,7 @@ class Batch_test : public beast::unit_test::suite // temARRAY_TOO_LARGE: Batch: signers array exceeds 8 entries. { - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -1472,7 +1478,7 @@ class Batch_test : public beast::unit_test::suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -1608,7 +1614,7 @@ class Batch_test : public beast::unit_test::suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -1840,7 +1846,7 @@ class Batch_test : public beast::unit_test::suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -2062,7 +2068,7 @@ class Batch_test : public beast::unit_test::suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -2248,14 +2254,26 @@ class Batch_test : public beast::unit_test::suite } void - testInnerSubmitRPC(FeatureBitset features) + doTestInnerSubmitRPC(FeatureBitset features, bool withBatch) { - testcase("inner submit rpc"); + bool const withInnerSigFix = features[fixBatchInnerSigs]; + + std::string const testName = [&]() { + std::stringstream ss; + ss << "inner submit rpc: batch " + << (withBatch ? "enabled" : "disabled") << ", inner sig fix: " + << (withInnerSigFix ? "enabled" : "disabled") << ": "; + return ss.str(); + }(); + + auto const amend = withBatch ? features : features - featureBatch; using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, amend}; + if (!BEAST_EXPECT(amend[featureBatch] == withBatch)) + return; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -2263,75 +2281,170 @@ class Batch_test : public beast::unit_test::suite env.fund(XRP(10000), alice, bob); env.close(); - auto submitAndValidate = [&](Slice const& slice) { - auto const jrr = env.rpc("submit", strHex(slice))[jss::result]; - BEAST_EXPECT( - jrr[jss::status] == "error" && - jrr[jss::error] == "invalidTransaction" && - jrr[jss::error_exception] == - "fails local checks: Malformed: Invalid inner batch " - "transaction."); - env.close(); - }; + auto submitAndValidate = + [&](std::string caseName, + Slice const& slice, + int line, + std::optional expectedEnabled = std::nullopt, + std::optional expectedDisabled = std::nullopt, + bool expectInvalidFlag = false) { + testcase << testName << caseName + << (expectInvalidFlag + ? " - Expected to reach tx engine!" + : ""); + auto const jrr = env.rpc("submit", strHex(slice))[jss::result]; + auto const expected = withBatch + ? expectedEnabled.value_or( + "fails local checks: Malformed: Invalid inner batch " + "transaction.") + : expectedDisabled.value_or( + "fails local checks: Empty SigningPubKey."); + if (expectInvalidFlag) + { + expect( + jrr[jss::status] == "success" && + jrr[jss::engine_result] == "temINVALID_FLAG", + pretty(jrr), + __FILE__, + line); + } + else + { + expect( + jrr[jss::status] == "error" && + jrr[jss::error] == "invalidTransaction" && + jrr[jss::error_exception] == expected, + pretty(jrr), + __FILE__, + line); + } + env.close(); + }; // Invalid RPC Submission: TxnSignature - // - has `TxnSignature` field + // + has `TxnSignature` field // - has no `SigningPubKey` field // - has no `Signers` field - // - has `tfInnerBatchTxn` flag + // + has `tfInnerBatchTxn` flag { auto txn = batch::inner(pay(alice, bob, XRP(1)), env.seq(alice)); txn[sfTxnSignature] = "DEADBEEF"; STParsedJSONObject parsed("test", txn.getTxn()); Serializer s; parsed.object->add(s); - submitAndValidate(s.slice()); + submitAndValidate("TxnSignature set", s.slice(), __LINE__); } // Invalid RPC Submission: SigningPubKey // - has no `TxnSignature` field - // - has `SigningPubKey` field + // + has `SigningPubKey` field // - has no `Signers` field - // - has `tfInnerBatchTxn` flag + // + has `tfInnerBatchTxn` flag { auto txn = batch::inner(pay(alice, bob, XRP(1)), env.seq(alice)); txn[sfSigningPubKey] = strHex(alice.pk()); STParsedJSONObject parsed("test", txn.getTxn()); Serializer s; parsed.object->add(s); - submitAndValidate(s.slice()); + submitAndValidate( + "SigningPubKey set", + s.slice(), + __LINE__, + std::nullopt, + "fails local checks: Invalid signature."); } // Invalid RPC Submission: Signers // - has no `TxnSignature` field - // - has empty `SigningPubKey` field - // - has `Signers` field - // - has `tfInnerBatchTxn` flag + // + has empty `SigningPubKey` field + // + has `Signers` field + // + has `tfInnerBatchTxn` flag { auto txn = batch::inner(pay(alice, bob, XRP(1)), env.seq(alice)); txn[sfSigners] = Json::arrayValue; STParsedJSONObject parsed("test", txn.getTxn()); Serializer s; parsed.object->add(s); - submitAndValidate(s.slice()); + submitAndValidate( + "Signers set", + s.slice(), + __LINE__, + std::nullopt, + "fails local checks: Invalid Signers array size."); + } + + { + // Fully signed inner batch transaction + auto const txn = + batch::inner(pay(alice, bob, XRP(1)), env.seq(alice)); + auto const jt = env.jt(txn.getTxn()); + + STParsedJSONObject parsed("test", jt.jv); + Serializer s; + parsed.object->add(s); + submitAndValidate( + "Fully signed", + s.slice(), + __LINE__, + std::nullopt, + std::nullopt, + !withBatch); } // Invalid RPC Submission: tfInnerBatchTxn // - has no `TxnSignature` field - // - has empty `SigningPubKey` field + // + has empty `SigningPubKey` field // - has no `Signers` field - // - has `tfInnerBatchTxn` flag + // + has `tfInnerBatchTxn` flag { auto txn = batch::inner(pay(alice, bob, XRP(1)), env.seq(alice)); STParsedJSONObject parsed("test", txn.getTxn()); Serializer s; parsed.object->add(s); - auto const jrr = env.rpc("submit", strHex(s.slice()))[jss::result]; - BEAST_EXPECT( - jrr[jss::status] == "success" && - jrr[jss::engine_result] == "temINVALID_FLAG"); + submitAndValidate( + "No signing fields set", + s.slice(), + __LINE__, + "fails local checks: Empty SigningPubKey.", + "fails local checks: Empty SigningPubKey.", + withBatch && !withInnerSigFix); + } - env.close(); + // Invalid RPC Submission: tfInnerBatchTxn pseudo-transaction + // - has no `TxnSignature` field + // + has empty `SigningPubKey` field + // - has no `Signers` field + // + has `tfInnerBatchTxn` flag + { + STTx amendTx( + ttAMENDMENT, [seq = env.closed()->header().seq + 1](auto& obj) { + obj.setAccountID(sfAccount, AccountID()); + obj.setFieldH256(sfAmendment, fixBatchInnerSigs); + obj.setFieldU32(sfLedgerSequence, seq); + obj.setFieldU32(sfFlags, tfInnerBatchTxn); + }); + auto txn = batch::inner( + amendTx.getJson(JsonOptions::none), env.seq(alice)); + STParsedJSONObject parsed("test", txn.getTxn()); + Serializer s; + parsed.object->add(s); + submitAndValidate( + "Pseudo-transaction", + s.slice(), + __LINE__, + withInnerSigFix + ? "fails local checks: Empty SigningPubKey." + : "fails local checks: Cannot submit pseudo transactions.", + "fails local checks: Empty SigningPubKey."); + } + } + + void + testInnerSubmitRPC(FeatureBitset features) + { + for (bool const withBatch : {true, false}) + { + doTestInnerSubmitRPC(features, withBatch); } } @@ -2343,7 +2456,7 @@ class Batch_test : public beast::unit_test::suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -2390,7 +2503,7 @@ class Batch_test : public beast::unit_test::suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -2443,7 +2556,7 @@ class Batch_test : public beast::unit_test::suite // tfIndependent: account delete success { - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -2484,7 +2597,7 @@ class Batch_test : public beast::unit_test::suite // tfIndependent: account delete fails { - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -2529,7 +2642,7 @@ class Batch_test : public beast::unit_test::suite // tfAllOrNothing: account delete fails { - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -2581,7 +2694,6 @@ class Batch_test : public beast::unit_test::suite test::jtx::Env env{ *this, - envconfig(), features | featureSingleAssetVault | featureLendingProtocol | featureMPTokensV1}; @@ -2776,7 +2888,7 @@ class Batch_test : public beast::unit_test::suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -2889,7 +3001,7 @@ class Batch_test : public beast::unit_test::suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -2947,7 +3059,7 @@ class Batch_test : public beast::unit_test::suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -3009,7 +3121,7 @@ class Batch_test : public beast::unit_test::suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -3058,7 +3170,7 @@ class Batch_test : public beast::unit_test::suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -3106,7 +3218,7 @@ class Batch_test : public beast::unit_test::suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -3169,7 +3281,7 @@ class Batch_test : public beast::unit_test::suite // overwritten by the payment in the batch transaction. Because the // terPRE_SEQ is outside of the batch this noop transaction will ge // reapplied in the following ledger - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; env.fund(XRP(10000), alice, bob, carol); env.close(); @@ -3216,7 +3328,7 @@ class Batch_test : public beast::unit_test::suite // IMPORTANT: The batch txn is applied first, then the noop txn. // Because of this ordering, the noop txn is not applied and is // overwritten by the payment in the batch transaction. - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; env.fund(XRP(10000), alice, bob); env.close(); @@ -3258,7 +3370,7 @@ class Batch_test : public beast::unit_test::suite // IMPORTANT: The batch txn is applied first, then the noop txn. // Because of this ordering, the noop txn is not applied and is // overwritten by the payment in the batch transaction. - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; env.fund(XRP(10000), alice, bob); env.close(); @@ -3295,7 +3407,7 @@ class Batch_test : public beast::unit_test::suite // Outer Batch terPRE_SEQ { - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; env.fund(XRP(10000), alice, bob, carol); env.close(); @@ -3353,7 +3465,7 @@ class Batch_test : public beast::unit_test::suite // IMPORTANT: The batch txn is applied first, then the noop txn. // Because of this ordering, the noop txn is not applied and is // overwritten by the payment in the batch transaction. - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; env.fund(XRP(10000), alice, bob); env.close(); @@ -3402,7 +3514,7 @@ class Batch_test : public beast::unit_test::suite // IMPORTANT: The batch txn is applied first, then the noop txn. // Because of this ordering, the noop txn is not applied and is // overwritten by the payment in the batch transaction. - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; env.fund(XRP(10000), alice, bob); env.close(); @@ -3464,7 +3576,7 @@ class Batch_test : public beast::unit_test::suite // batch will run in the close ledger process. The batch will be // allied and then retry this transaction in the current ledger. - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; env.fund(XRP(10000), alice, bob); env.close(); @@ -3511,7 +3623,7 @@ class Batch_test : public beast::unit_test::suite // Create Object Before Batch Txn { - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; env.fund(XRP(10000), alice, bob); env.close(); @@ -3558,7 +3670,7 @@ class Batch_test : public beast::unit_test::suite // batch will run in the close ledger process. The batch will be // applied and then retry this transaction in the current ledger. - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; env.fund(XRP(10000), alice, bob); env.close(); @@ -3605,7 +3717,7 @@ class Batch_test : public beast::unit_test::suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -3644,7 +3756,7 @@ class Batch_test : public beast::unit_test::suite using namespace test::jtx; using namespace std::literals; - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; XRPAmount const baseFee = env.current()->fees().base; auto const alice = Account("alice"); @@ -3725,6 +3837,7 @@ class Batch_test : public beast::unit_test::suite *this, makeSmallQueueConfig( {{"minimum_txn_in_ledger_standalone", "2"}}), + features, nullptr, beast::severities::kError}; @@ -3785,6 +3898,7 @@ class Batch_test : public beast::unit_test::suite *this, makeSmallQueueConfig( {{"minimum_txn_in_ledger_standalone", "2"}}), + features, nullptr, beast::severities::kError}; @@ -3892,7 +4006,7 @@ class Batch_test : public beast::unit_test::suite // delegated non atomic inner { - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -3937,7 +4051,7 @@ class Batch_test : public beast::unit_test::suite // delegated atomic inner { - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -3989,7 +4103,7 @@ class Batch_test : public beast::unit_test::suite // this also makes sure tfInnerBatchTxn won't block delegated AccountSet // with granular permission { - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); @@ -4038,7 +4152,7 @@ class Batch_test : public beast::unit_test::suite // this also makes sure tfInnerBatchTxn won't block delegated // MPTokenIssuanceSet with granular permission { - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; Account alice{"alice"}; Account bob{"bob"}; env.fund(XRP(100000), alice, bob); @@ -4094,7 +4208,7 @@ class Batch_test : public beast::unit_test::suite // this also makes sure tfInnerBatchTxn won't block delegated TrustSet // with granular permission { - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; Account gw{"gw"}; Account alice{"alice"}; Account bob{"bob"}; @@ -4134,7 +4248,7 @@ class Batch_test : public beast::unit_test::suite // inner transaction not authorized by the delegating account. { - test::jtx::Env env{*this, envconfig()}; + test::jtx::Env env{*this, features}; Account gw{"gw"}; Account alice{"alice"}; Account bob{"bob"}; @@ -4182,7 +4296,7 @@ class Batch_test : public beast::unit_test::suite testcase("Validate RPC response"); using namespace jtx; - Env env(*this); + Env env(*this, features); Account const alice("alice"); Account const bob("bob"); env.fund(XRP(10000), alice, bob); @@ -4259,7 +4373,7 @@ class Batch_test : public beast::unit_test::suite testBatchCalculateBaseFee(FeatureBitset features) { using namespace jtx; - Env env(*this); + Env env(*this, features); Account const alice("alice"); Account const bob("bob"); Account const carol("carol"); @@ -4384,6 +4498,7 @@ public: { using namespace test::jtx; auto const sa = testable_amendments(); + testWithFeats(sa - fixBatchInnerSigs); testWithFeats(sa); } }; diff --git a/src/xrpld/app/misc/NetworkOPs.cpp b/src/xrpld/app/misc/NetworkOPs.cpp index 6a00354b15..2422ec4ae6 100644 --- a/src/xrpld/app/misc/NetworkOPs.cpp +++ b/src/xrpld/app/misc/NetworkOPs.cpp @@ -1681,7 +1681,7 @@ NetworkOPsImp::apply(std::unique_lock& batchLock) // only be set if the Batch feature is enabled. If Batch is // not enabled, the flag is always invalid, so don't relay // it regardless. - !sttx.isFlag(tfInnerBatchTxn)) + !(sttx.isFlag(tfInnerBatchTxn))) { protocol::TMTransaction tx; Serializer s; diff --git a/src/xrpld/app/tx/detail/Transactor.cpp b/src/xrpld/app/tx/detail/Transactor.cpp index 851712fe90..a834f7c6c3 100644 --- a/src/xrpld/app/tx/detail/Transactor.cpp +++ b/src/xrpld/app/tx/detail/Transactor.cpp @@ -204,8 +204,14 @@ Transactor::preflight2(PreflightContext const& ctx) // regardless of success or failure return *ret; + // It should be impossible for the InnerBatchTxn flag to be set without + // featureBatch being enabled + XRPL_ASSERT_PARTS( + !ctx.tx.isFlag(tfInnerBatchTxn) || ctx.rules.enabled(featureBatch), + "xrpl::Transactor::preflight2", + "InnerBatch flag only set if feature enabled"); // Skip signature check on batch inner transactions - if (ctx.tx.isFlag(tfInnerBatchTxn) && !ctx.rules.enabled(featureBatch)) + if (ctx.tx.isFlag(tfInnerBatchTxn) && ctx.rules.enabled(featureBatch)) return tesSUCCESS; // Do not add any checks after this point that are relevant for // batch inner transactions. They will be skipped. diff --git a/src/xrpld/app/tx/detail/apply.cpp b/src/xrpld/app/tx/detail/apply.cpp index 5209c46f8f..a75f0cc967 100644 --- a/src/xrpld/app/tx/detail/apply.cpp +++ b/src/xrpld/app/tx/detail/apply.cpp @@ -41,15 +41,22 @@ checkValidity( Validity::SigBad, "Malformed: Invalid inner batch transaction."}; - std::string reason; - if (!passesLocalChecks(tx, reason)) + // This block should probably have never been included in the + // original `Batch` implementation. An inner transaction never + // has a valid signature. + bool const neverValid = rules.enabled(fixBatchInnerSigs); + if (!neverValid) { - router.setFlags(id, SF_LOCALBAD); - return {Validity::SigGoodOnly, reason}; - } + std::string reason; + if (!passesLocalChecks(tx, reason)) + { + router.setFlags(id, SF_LOCALBAD); + return {Validity::SigGoodOnly, reason}; + } - router.setFlags(id, SF_SIGGOOD); - return {Validity::Valid, ""}; + router.setFlags(id, SF_SIGGOOD); + return {Validity::Valid, ""}; + } } if (any(flags & SF_SIGBAD)) From 92d40de4cbb787474f90d6aa6a959963f8299992 Mon Sep 17 00:00:00 2001 From: Bart Date: Mon, 12 Jan 2026 12:53:46 -0500 Subject: [PATCH 11/11] chore: Pin pre-commit hooks to commit hashes (#6205) This change updates and pins the Black and CSpell pre-commit hooks. --- .config/cspell.config.yaml | 4 ++++ .pre-commit-config.yaml | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/.config/cspell.config.yaml b/.config/cspell.config.yaml index edcdcc92ba..8f782d9960 100644 --- a/.config/cspell.config.yaml +++ b/.config/cspell.config.yaml @@ -69,6 +69,7 @@ words: - cryptoconditional - cryptoconditions - csprng + - ctest - ctid - currenttxhash - daria @@ -104,6 +105,7 @@ words: - iou - ious - isrdc + - itype - jemalloc - jlog - keylet @@ -192,10 +194,12 @@ words: - roundings - sahyadri - Satoshi + - scons - secp - sendq - seqit - sf + - SFIELD - shamap - shamapitem - sidechain diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 00bec32ed6..603cf39375 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -32,12 +32,12 @@ repos: - id: prettier - repo: https://github.com/psf/black-pre-commit-mirror - rev: 25.11.0 + rev: 831207fd435b47aeffdf6af853097e64322b4d44 # frozen: v25.12.0 hooks: - id: black - repo: https://github.com/streetsidesoftware/cspell-cli - rev: v9.2.0 + rev: 1cfa010f078c354f3ffb8413616280cc28f5ba21 # frozen: v9.4.0 hooks: - id: cspell # Spell check changed files exclude: .config/cspell.config.yaml