Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Unit test that sign_for returns a correct hash (RIPD-1583) #2333

Closed
wants to merge 1 commit into from
Closed

Unit test that sign_for returns a correct hash (RIPD-1583) #2333

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 75 additions & 0 deletions src/test/app/MultiSign_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1134,6 +1134,80 @@ class MultiSign_test : public beast::unit_test::suite
env.close();
}

void test_signForHash()
{
// Make sure that the "hash" field returned by the "sign_for" RPC
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only the last sign_for before submitting returns the "right" hash huh? In general, each signer doesn't know the hash to look out for though right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. Each added signature changes the hash. But if the transaction with those signatures is submitted, either as a blob through submit or as text through submit_multisigned, then the hash does not change.

As a use model, it is probably smartest to rely on the hash returned when the transaction is submitted. But since we are returning a hash value on signing, the value we return should be correct.

// command matches the hash returned when that command is sent
// through "submit_multisigned". Make sure that hash also locates
// the transaction in the ledger.
using namespace jtx;
Account const alice {"alice", KeyType::ed25519};

Env env(*this);
env.fund (XRP(1000), alice);
env.close();

env (signers (alice, 2, {{bogie, 1}, {ghost, 1}}));
env.close();

// Use sign_for to sign a transaction where alice pays 10 XRP to
// masterpassphrase.
std::uint32_t baseFee = env.current()->fees().base;
Json::Value jvSig1;
jvSig1[jss::account] = bogie.human();
jvSig1[jss::secret] = bogie.name();
jvSig1[jss::tx_json][jss::Account] = alice.human();
jvSig1[jss::tx_json][jss::Amount] = 10000000;
jvSig1[jss::tx_json][jss::Destination] = env.master.human();
jvSig1[jss::tx_json][jss::Fee] = 3 * baseFee;
jvSig1[jss::tx_json][jss::Sequence] = env.seq(alice);
jvSig1[jss::tx_json][jss::TransactionType] = "Payment";

Json::Value jvSig2 = env.rpc (
"json", "sign_for", to_string (jvSig1));
BEAST_EXPECT (
jvSig2[jss::result][jss::status].asString() == "success");

// Save the hash with one signature for use later.
std::string const hash1 =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth checking any of the hash characteristics here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly? To me the important characteristic of the hash is it that it allows me to look up the transaction in the ledger. So the various hash checks I'm doing throughout the test show three things:

  1. Adding the first signature provides us a hash. When we add the second signature, that also provides a hash. Those two hashes should be different. It would be easy to say, well of course they are different. But the ripple-online-tool had a bug where they were not different. So it's worth checking.
  2. If we submit the transaction with two signatures, the hash that comes back from that submission should be the same as the hash that we got back when we signed the second time.
  3. The hash returned when we submitted the transaction (which is the same as the hash returned when we signed the second time) should be able to locate the transaction in the ledger.

Are there other characteristics I should be testing for? Thanks.

jvSig2[jss::result][jss::tx_json][jss::hash].asString();

// Add the next signature and sign again.
jvSig2[jss::result][jss::account] = ghost.human();
jvSig2[jss::result][jss::secret] = ghost.name();
Json::Value jvSubmit = env.rpc (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Json::Value jvSubmit might be better named Json::Value jvSig3.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually used jvSig3 initially. I changed the name because the JSON in jvSubmit is what we'll submit to the network (yeah, it's a unit test, so it's a network of one) on line 1190. So that's my justification for the variable name...

"json", "sign_for", to_string (jvSig2[jss::result]));
BEAST_EXPECT (
jvSubmit[jss::result][jss::status].asString() == "success");

// Save the hash with two signatures for use later.
std::string const hash2 =
jvSubmit[jss::result][jss::tx_json][jss::hash].asString();
BEAST_EXPECT (hash1 != hash2);

// Submit the result of the two signatures.
Json::Value jvResult = env.rpc (
"json", "submit_multisigned", to_string (jvSubmit[jss::result]));
BEAST_EXPECT (
jvResult[jss::result][jss::status].asString() == "success");
BEAST_EXPECT (jvResult[jss::result]
[jss::engine_result].asString() == "tesSUCCESS");

// The hash from the submit should be the same as the hash from the
// second signing.
BEAST_EXPECT (
hash2 == jvResult[jss::result][jss::tx_json][jss::hash].asString());
env.close();

// The transaction we just submitted should now be available and
// validated.
Json::Value jvTx = env.rpc ("tx", hash2);
BEAST_EXPECT (jvTx[jss::result][jss::status].asString() == "success");
BEAST_EXPECT (jvTx[jss::result][jss::validated].asString() == "true");
BEAST_EXPECT (jvTx[jss::result][jss::meta]
[sfTransactionResult.jsonName].asString() == "tesSUCCESS");
}

void run() override
{
test_noReserve();
Expand All @@ -1152,6 +1226,7 @@ class MultiSign_test : public beast::unit_test::suite
test_badSignatureText();
test_noMultiSigners();
test_multisigningMultisigner();
test_signForHash();
}
};

Expand Down