test: introduce basic unit tests for instantsend #6606
Conversation
WalkthroughThe changes introduce a new test file, Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
src/test/evo_islock_tests.cpp
Outdated
| // Now add two dummy inputs to the lock | ||
| islock.inputs.clear(); | ||
| // Construct two dummy outpoints (using uint256S for a dummy hash) | ||
| COutPoint op1(uint256S("0x0000000000000000000000000000000000000000000000000000000000000001"), 0); |
There was a problem hiding this comment.
nit: coule use uint256::ONE and uint256::TWO here
src/test/evo_islock_tests.cpp
Outdated
| CHashWriter hw(SER_GETHASH, 0); | ||
| hw << std::string_view("islock"); | ||
| hw << islock.inputs; | ||
| const uint256 expected = hw.GetHash(); |
There was a problem hiding this comment.
nit: refactor it to a function so far as it is used twice (see below)
src/test/evo_islock_tests.cpp
Outdated
| BOOST_CHECK_EQUAL(input.n, expectedInputN); | ||
|
|
||
| // Compute the expected request ID: it is the hash of the constant prefix "islock" followed by the inputs. | ||
| CHashWriter hw(SER_GETHASH, 0); |
There was a problem hiding this comment.
nit: refactor it to a function so far as it is used twice
src/test/evo_islock_tests.cpp
Outdated
| ss >> islock; | ||
|
|
||
| // Verify the calculated signHash | ||
| auto signHash = llmq::BuildSignHash(Consensus::LLMQType::LLMQ_60_75, uint256S(quorumHash), islock.GetRequestId(), islock.txid); |
There was a problem hiding this comment.
let's add one more unit test with other quorum type to be sure that this argument is used?
I think a check such as BOOST_CHECK(signHash.ToString() != expectedSignHash) is enough.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/test/evo_islock_tests.cpp (3)
69-69: Document the BLS scheme settingSetting
bls::bls_legacy_scheme.store(false)is critical for the test to work, but the reasoning behind this choice could be better documented. Consider adding a more detailed comment explaining why this setting is necessary and what would happen if it were set to true.
58-59: DefinequorumHashconsistently with other variablesWhile most string variables use
std::string_view,quorumHashis defined asstd::string. For consistency, consider usingstd::string_viewfor all string constants or provide a comment explaining why this particular variable needs to be astd::string.
1-104: Overall test coverage is comprehensive but could be expandedThe implementation provides good basic coverage of the InstantSend functionality, with tests for request ID calculation and deserialization from real data. To further improve coverage, consider:
- Adding tests with different quorum types as previously suggested
- Testing the serialization process (not just deserialization)
- Adding tests for edge cases or error conditions
These tests provide a solid foundation but expanding them would further enhance the robustness of the codebase.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 13-13: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/evo_islock_tests.cpp(1 hunks)
🧰 Additional context used
🪛 Cppcheck (2.10-2)
src/test/evo_islock_tests.cpp
[error] 13-13: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: arm-linux-build / Build source
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
🔇 Additional comments (6)
src/test/evo_islock_tests.cpp (6)
15-21: Helper function well implementedThe
CalculateRequestIdfunction is cleanly implemented to mirror the internal functionality ofCInstantSendLock::GetRequestId(). This is good practice for unit tests as it provides an independent verification mechanism.
23-48: Test case covers empty and non-empty inputs correctlyThe
getrequestidtest case appropriately verifies the request ID calculation for both an empty inputs vector and one with dummy inputs. Good use ofuint256::ONEanduint256::TWOfor creating test outpoints.
40-42: Dummy outpoints correctly constructedGood use of the predefined constants
uint256::ONEanduint256::TWOfor creating test outpoints rather than manually constructing arbitrary values.
50-102: Consider adding a test with different quorum typesThe deserialization test is thorough, verifying multiple aspects of the deserialized data including hashes, transaction ID, cycle hash, inputs, and signature. However, as previously suggested, it would be beneficial to add a test with a different quorum type to ensure that parameter is properly handled.
You could add a variation of the current test where you use a different quorum type and verify that the signature hash changes, with something like:
// Test with a different quorum type auto signHashAlt = llmq::BuildSignHash(Consensus::LLMQType::LLMQ_400_60, uint256S(quorumHash), islock.GetRequestId(), islock.txid); BOOST_CHECK(signHashAlt != signHash);
77-80: Add test for different quorum typesThis section tests only one quorum type (
LLMQ_60_75). As previously suggested, it would be valuable to add a test case with a different quorum type to verify that theBuildSignHashfunction properly handles different quorum types.
97-98: Good verification of request ID calculationThe test correctly verifies that the calculated request ID matches the expected value derived from the inputs. This ensures consistency between the internal implementation and the test's expectations.
|
pls consider 98b20c8 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/evo_islock_tests.cpp (1)
67-67: Consider restoring the BLS legacy scheme setting.The test changes the BLS legacy scheme setting but doesn't restore it afterward. This could potentially affect other tests that rely on a different setting.
+ bool original_legacy_scheme = bls::bls_legacy_scheme.load(); bls::bls_legacy_scheme.store(false); + // Scope cleanup or defer + struct BLSLegacySchemeGuard { + bool original_value; + BLSLegacySchemeGuard(bool val) : original_value(val) {} + ~BLSLegacySchemeGuard() { bls::bls_legacy_scheme.store(original_value); } + } guard(original_legacy_scheme);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/evo_islock_tests.cpp(1 hunks)
🧰 Additional context used
🪛 Cppcheck (2.10-2)
src/test/evo_islock_tests.cpp
[error] 12-12: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (6)
src/test/evo_islock_tests.cpp (6)
14-20: Good implementation of the helper function.Nice work creating a clean helper function for calculating the request ID. This function is reused across multiple test cases, which promotes code reuse and maintainability.
39-40: Good use of predefined constants.You've implemented the previous review suggestion by using
uint256::ONEanduint256::TWOinstead of manually creating these values.
22-47: Well-structured test for GetRequestId functionality.The test case properly validates the
GetRequestIdmethod with both empty and populated inputs vectors. This ensures the hash calculation works correctly in both scenarios.
49-100: Comprehensive deserialization test with real-world data.Excellent test case that uses real-world data to verify the deserialization of an InstantSend lock. The test covers all important fields and validates the relationship between the inputs and the request ID.
1-102: Good implementation of the InstantSend test suite.Overall, this is a well-written test suite that covers important functionality of the InstantSend feature. The tests are detailed, use real-world data where appropriate, and ensure that critical functionality works as expected.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 12-12: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
76-78:❓ Verification inconclusive
Consider adding a test with a different quorum type.
A previous review suggested adding a test with a different quorum type to verify that the quorum type argument is properly used. This would ensure that
BuildSignHashhandles different quorum types correctly.
🏁 Script executed:
#!/bin/bash # Check if there are other tests for different quorum types ast-grep --pattern $'BuildSignHash\(Consensus::LLMQType::$_, $_'Length of output: 231
Action: Expand test coverage for BuildSignHash quorum types
The current test in
src/test/evo_islock_tests.cppusesConsensus::LLMQType::LLMQ_60_75exclusively. Our initial search (filtered to avoid non-UTF-8 files) did not reveal any tests that use an alternative quorum type. To ensure that the quorum type argument is correctly handled byBuildSignHash, please add a test case using a different quorum type.Verify manually that no such additional tests exist in your codebase, and once added, confirm that the function behaves as expected with multiple quorum types.
Co-authored-by: UdjinM6 <[email protected]>
Co-authored-by: UdjinM6 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/test/evo_islock_tests.cpp (2)
36-43: Consider removing redundantclear()callLine 37 calls
islock.inputs.clear()immediately after creating an empty InstantSendLock object, which already has an empty inputs vector.// Now add two dummy inputs to the lock - islock.inputs.clear(); // Construct two dummy outpoints (using uint256S for a dummy hash) COutPoint op1(uint256::ONE, 0); COutPoint op2(uint256::TWO, 1); islock.inputs.push_back(op1); islock.inputs.push_back(op2);
68-68: Restore global state after modifying bls_legacy_schemeThis line changes a global state that could potentially affect other tests. Consider restoring it to its original value at the end of the test.
// This islock was created with non-legacy. Using legacy will result in the signature being all zeros. bls::bls_legacy_scheme.store(false); + // Save original value to restore later + const bool original_bls_legacy_scheme = bls::bls_legacy_scheme.load(); + bls::bls_legacy_scheme.store(false); + + // ... rest of the test ... + + // Restore original value + bls::bls_legacy_scheme.store(original_bls_legacy_scheme);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/evo_islock_tests.cpp(1 hunks)
🧰 Additional context used
🪛 Cppcheck (2.10-2)
src/test/evo_islock_tests.cpp
[error] 12-12: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (5)
src/test/evo_islock_tests.cpp (5)
1-11: Well-structured includes with clear organizationThe includes are appropriately selected for the testing requirements, with a helpful comment explaining the purpose of
util/strencodings.hfor constructing dummy outpoints.
14-20: Good extraction of common logic into a helper functionThis helper function cleanly encapsulates the hash calculation logic that's used in multiple test cases. It follows the same logic as the implementation being tested while keeping the test code DRY.
77-79: Add test with a different quorum typeAs mentioned in a previous review, it would be beneficial to verify that the quorum type argument is correctly used in the
BuildSignHashfunction by adding a test with a different quorum type.// Verify the calculated signHash auto signHash = llmq::BuildSignHash(Consensus::LLMQType::LLMQ_60_75, uint256S(quorumHashStr), islock.GetRequestId(), islock.txid); BOOST_CHECK_EQUAL(signHash.ToString(), expectedSignHashStr); + + // Verify that a different quorum type produces a different signHash + auto differentSignHash = llmq::BuildSignHash(Consensus::LLMQType::LLMQ_400_60, uint256S(quorumHashStr), islock.GetRequestId(), + islock.txid); + BOOST_CHECK(signHash != differentSignHash);
96-97: Consider reusing the CalculateRequestId helper function in both test casesThis is a good example of code reuse between test cases. The helper function ensures consistency in how the expected request ID is calculated.
49-101: Comprehensive test for InstantSend lock deserializationThis test provides thorough validation of the deserialization process using real-world data. It verifies multiple aspects of the deserialized lock (signHash, txid, cycleHash, inputs, requestId, signature) which is excellent for ensuring the code works correctly with actual network data.
What was done?
Introduce unit tests for some instantsend logic
How Has This Been Tested?
Running tests
Breaking Changes
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.