Shardstore Unittest Fix #3855

undertome · 2021-05-25T17:40:42Z

High Level Overview of Change

This fixes some recent failures in the shard database unit tests.

Context of Change

The unit test functions testDeterministicShard and testImportWithOnlineDelete were modified to fix test case failures. Please limit the review to 7778710 as this fix is stacked onto several unmerged PRs.

Type of Change

[x ] Bug fix (non-breaking change which fixes an issue)

Typically, an RPC response contains a `result` field, which contains details about the operation performed. For ease of parsing, forwarded responses must look like a non-forwarded response. In some instances the response was incorrectly composed, so that the actual `result` object would be encapsulated by an outer `result` object, breaking existing code. This commit, addresses this issue and correctly "folds" the `result` field, ensuring a consistent schema for responses.

The `tx` command supports output in both "text" and "binary" modes, controlled by the binary flag. For more details on the command and the possible arguments, please see: https://xrpl.org/tx.html. The existing handler would incorrectly deal with metadata when in binary mode. This commit corrects this issue, ensuring that the metadata is properly encoded, depending on the mode.

In order to effectively mitigate CVE-2021-3499 even when compiling against versions of OpenSSL prior to 1.1.1k, this commit: 1) requires use of TLS 1.2 or later. Note that both TLS 1.0 and TLS 1.1 have been officially deprecated for over a year. 2) disables renegotiation support for TLS 1.2 connections. Lastly, this commit also changes the default list of ciphers that the server offers, limiting it only to ciphers that are part of TLS 1.2.

* load_factor was missing from server_info when the server was running in reporting mode. Now, the reporting mode server calls server_info on the p2p node, and propagates the load_factor back to the client.

Substantial reductions in an offer's effective quality from its initial quality may clog offer books.

* The std::less and std::equal_to comparators have [[nodiscard]], which conflicts with boost::bimap validations.

scottschurr · 2021-05-25T21:16:02Z

🎉

miguelportilla · 2021-05-27T19:23:08Z

src/test/nodestore/DatabaseShard_test.cpp

+            BEAST_EXPECT(
+                ripemd160File((path / "nudb.key").string()) == ripemd160Key);
+            BEAST_EXPECT(
+                ripemd160File((path / "nudb.dat").string()) == ripemd160Dat);


I don't think these two checks are necessary here as we perform them at the end of the loop.

miguelportilla · 2021-05-27T19:24:02Z

src/test/nodestore/DatabaseShard_test.cpp

@@ -1140,6 +1135,20 @@ class DatabaseShard_test : public TestBase
                if (createShard(data, *db) < 0)
                    return;
            }
+


The createShard call should be

if (!BEAST_EXPECT(createShard(data, *db) != std::nullopt)) return;

Don't change anything, but this could also be written as

if (!BEAST_EXPECT(createShard(data, *db))) return;

or

if (!BEAST_EXPECT(createShard(data, *db).has_value())) return;

miguelportilla · 2021-05-27T19:26:42Z

src/test/nodestore/DatabaseShard_test.cpp

+                ripemd160File((path / "nudb.key").string()) == ripemd160Key);
+            BEAST_EXPECT(
+                ripemd160File((path / "nudb.dat").string()) == ripemd160Dat);
+


The waitShard call below should be

if (!BEAST_EXPECT(waitShard(*db, 1) != std::nullopt)) return;

miguelportilla · 2021-05-27T19:29:58Z

src/test/nodestore/DatabaseShard_test.cpp

@@ -1246,22 +1254,28 @@ class DatabaseShard_test : public TestBase
            Database& ndb = env.app().getNodeStore();
            BEAST_EXPECT(db);

-            auto const shardCount = 4;
+            // Create some ledgers for the shard store to
+            // import


Nit. This comment can be a single line

miguelportilla · 2021-05-27T19:45:01Z

src/test/nodestore/DatabaseShard_test.cpp

-            auto& store = env.app().getSHAMapStore();
-            auto lastRotated = store.getLastRotated();
+            // Enable online deletion now that the import has
+            // started


Nit. This comment can be a single line

miguelportilla · 2021-05-27T19:55:47Z

src/test/nodestore/DatabaseShard_test.cpp

@@ -1273,6 +1287,7 @@ class DatabaseShard_test : public TestBase
                    {
                        // A rotation occurred during shard import. Not
                        // necessarily an error
+


Nit: sequence can be renamed to ledgerSeq and be const

miguelportilla · 2021-05-27T20:05:07Z

src/test/nodestore/DatabaseShard_test.cpp

@@ -1281,7 +1296,8 @@ class DatabaseShard_test : public TestBase
                }
            });

-            data = TestData(seedValue * 2, 4, shardCount);
+            // Create more ledgers to trigger online deletion
+            data = TestData(seedValue * 2, 4, 1);


Due to the defaults, can also be

TestData(seedValue * 2);

Good point - done

miguelportilla

I left some nit comments but otherwise looks good. 👍

* Run the import process in a background thread * Prevent online_delete from removing ledgers pending import

* Create SQLite database for mapping transaction IDs to shard indexes * Create SQLite database for mapping ledger hashes to shard indexes * Create additional test cases for the shard database

* resolves XRPLF#3782 * gcc 8 is required for the charconv include file

ximinez

I know this is already merged, but I did look through it and everything looks good.

ximinez · 2021-06-04T18:55:41Z

src/test/nodestore/DatabaseShard_test.cpp

@@ -1140,6 +1135,20 @@ class DatabaseShard_test : public TestBase
                if (createShard(data, *db) < 0)
                    return;
            }
+


Don't change anything, but this could also be written as

if (!BEAST_EXPECT(createShard(data, *db))) return;

or

if (!BEAST_EXPECT(createShard(data, *db).has_value())) return;

mtrippled and others added 10 commits April 1, 2021 10:40

Support HTTP health check in reporting mode.

9c8cadd

Set version to 1.7.1

9106a06

Add load_factor to server_info in reporting mode

430802c

* load_factor was missing from server_info when the server was running in reporting mode. Now, the reporting mode server calls server_info on the p2p node, and propagates the load_factor back to the client.

Rm some offers where the quality is reduced:

1bb99e5

Substantial reductions in an offer's effective quality from its initial quality may clog offer books.

Update version of vcpkg for Windows builds

36fe196

Create comparator wrapper classes for MSVC 2019 build issues:

30fd458

* The std::less and std::equal_to comparators have [[nodiscard]], which conflicts with boost::bimap validations.

Set version to 1.7.2

34ee4ca

undertome requested review from miguelportilla and ximinez May 25, 2021 17:40

scottschurr assigned miguelportilla and ximinez May 25, 2021

manojsdoshi and others added 3 commits May 26, 2021 14:05

Merge master (1.7.2) into develop (1.8.0-b2)

c10c0be

Merge stoppable PR

b8a5834

Merge dead code PR

5ac56e7

miguelportilla reviewed May 27, 2021

View reviewed changes

miguelportilla approved these changes May 27, 2021

View reviewed changes

undertome and others added 4 commits May 27, 2021 14:21

Improve NodeStore to ShardStore imports

8cb98a2

* Run the import process in a background thread * Prevent online_delete from removing ledgers pending import

Extend peer shard info

91d7fea

Add lookup DBs to the Relational DB Interface

0e08e30

* Create SQLite database for mapping transaction IDs to shard indexes * Create SQLite database for mapping ledger hashes to shard indexes * Create additional test cases for the shard database

Update doc specifying min gcc version:

e714bec

* resolves XRPLF#3782 * gcc 8 is required for the charconv include file

pwang200 and others added 3 commits June 1, 2021 12:25

Add higher bound to validations to keep

dad4d19

Support dns in gRPC connection for reporting ETL

642905e

Advance shard store unittests

0e7d5cc

undertome force-pushed the shardstore-unittest-fix branch from a13cd9b to 0e7d5cc Compare June 1, 2021 19:40

This was referenced Jun 2, 2021

Proposed 1.8.0-b3 #3857

Closed

Proposed 1.8.0-b3 #3858

Merged

manojsdoshi closed this in #3858 Jun 4, 2021

ximinez reviewed Jun 4, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shardstore Unittest Fix #3855

Shardstore Unittest Fix #3855

undertome commented May 25, 2021

scottschurr commented May 25, 2021

miguelportilla May 27, 2021 •

edited

Loading

undertome May 27, 2021

miguelportilla May 27, 2021 •

edited

Loading

undertome May 27, 2021

ximinez Jun 4, 2021

miguelportilla May 27, 2021

undertome May 27, 2021

miguelportilla May 27, 2021

undertome May 27, 2021

miguelportilla May 27, 2021

undertome May 27, 2021

miguelportilla May 27, 2021 •

edited

Loading

undertome May 27, 2021

miguelportilla May 27, 2021 •

edited

Loading

undertome May 27, 2021

miguelportilla left a comment

ximinez left a comment

ximinez Jun 4, 2021

Shardstore Unittest Fix #3855

Shardstore Unittest Fix #3855

Conversation

undertome commented May 25, 2021

High Level Overview of Change

Context of Change

Type of Change

scottschurr commented May 25, 2021

miguelportilla May 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguelportilla May 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguelportilla May 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguelportilla May 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguelportilla left a comment

Choose a reason for hiding this comment

ximinez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguelportilla May 27, 2021 •

edited

Loading

miguelportilla May 27, 2021 •

edited

Loading

miguelportilla May 27, 2021 •

edited

Loading

miguelportilla May 27, 2021 •

edited

Loading