fix(framework): count pq_auth_sig in multisign fee and reverify checks#40
Open
bladehan1 wants to merge 3 commits into
Open
fix(framework): count pq_auth_sig in multisign fee and reverify checks#40bladehan1 wants to merge 3 commits into
bladehan1 wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthrough
ChangesPost-Quantum Signature Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
framework/src/test/java/org/tron/core/db/ManagerTest.java (1)
255-260: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake charged-fee assertions non-vacuous.
These “charged” tests can still pass if
multiSignFeeis0, even when charging is accidentally skipped. Pin a positive fee in test setup (and restore it) so the condition regression is always detected.Also applies to: 276-281
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/test/java/org/tron/core/db/ManagerTest.java` around lines 255 - 260, The assertions for multiSignFee charging at lines 255-260 can vacuously pass if the fee value is zero, even when the charging logic is broken. Before running the test that calls consumeMultiSignFee, explicitly set the multiSignFee to a positive hardcoded value using dbManager.getDynamicPropertiesStore().setMultiSignFee() in the test setup, and ensure it is restored to its original value after the test completes. This ensures the balance reduction and receipt assertions will actually validate that charging occurred rather than passing when fee equals zero. Apply the same pattern to the other charged-fee test referenced at lines 276-281.framework/src/test/java/org/tron/core/WalletTest.java (1)
1675-1679: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the actual hybrid signer addresses.
Line 1678 only checks the count, so this test could still pass if the PQ branch appended the wrong address. Assert both ECDSA and PQ approvals are present.
Suggested assertion tightening
assertEquals(GrpcAPI.TransactionApprovedList.Result.response_code.SUCCESS, reply.getResult().getCode()); assertEquals(2, reply.getApprovedListCount()); + Assert.assertTrue(reply.getApprovedListList() + .contains(ByteString.copyFrom(ecKey.getAddress()))); + Assert.assertTrue(reply.getApprovedListList() + .contains(ByteString.copyFrom(pqSignerAddr)));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/test/java/org/tron/core/WalletTest.java` around lines 1675 - 1679, The test method currently only asserts that the approved list count equals 2, but does not validate the actual addresses contained in that list. After the assertEquals check for getApprovedListCount() returning 2, add additional assertions to verify that both the ECDSA and PQ hybrid signer addresses are present in the reply's approved list by iterating through the approved list items and confirming the expected addresses match the actual returned addresses.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/src/test/java/org/tron/core/WalletTest.java`:
- Around line 1682-1683: The test method testApprovedListPqNotActivated()
currently only disables the FN_DSA_512 scheme via saveAllowFnDsa512(0L), but
since Wallet gates on isAnyPqSchemeAllowed(), other enabled schemes like
ML_DSA_44 will cause the test to not properly exercise the "no post-quantum
scheme activated" path. Disable all registered post-quantum schemes in this test
by calling the appropriate save methods on
chainBaseManager.getDynamicPropertiesStore() for each PQ scheme (not just
FN_DSA_512) to ensure no post-quantum scheme is enabled before testing the
inactive gate scenario.
---
Nitpick comments:
In `@framework/src/test/java/org/tron/core/db/ManagerTest.java`:
- Around line 255-260: The assertions for multiSignFee charging at lines 255-260
can vacuously pass if the fee value is zero, even when the charging logic is
broken. Before running the test that calls consumeMultiSignFee, explicitly set
the multiSignFee to a positive hardcoded value using
dbManager.getDynamicPropertiesStore().setMultiSignFee() in the test setup, and
ensure it is restored to its original value after the test completes. This
ensures the balance reduction and receipt assertions will actually validate that
charging occurred rather than passing when fee equals zero. Apply the same
pattern to the other charged-fee test referenced at lines 276-281.
In `@framework/src/test/java/org/tron/core/WalletTest.java`:
- Around line 1675-1679: The test method currently only asserts that the
approved list count equals 2, but does not validate the actual addresses
contained in that list. After the assertEquals check for getApprovedListCount()
returning 2, add additional assertions to verify that both the ECDSA and PQ
hybrid signer addresses are present in the reply's approved list by iterating
through the approved list items and confirming the expected addresses match the
actual returned addresses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 175bc10d-5126-40af-9d14-78bd040626d1
📒 Files selected for processing (5)
framework/src/main/java/org/tron/core/Wallet.javaframework/src/main/java/org/tron/core/db/Manager.javaframework/src/test/java/org/tron/core/WalletTest.javaframework/src/test/java/org/tron/core/actuator/utils/ProposalUtilTest.javaframework/src/test/java/org/tron/core/db/ManagerTest.java
Comment on lines
+1682
to
+1683
| public void testApprovedListPqNotActivated() throws Exception { | ||
| chainBaseManager.getDynamicPropertiesStore().saveAllowFnDsa512(0L); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Disable every PQ scheme for the inactive-gate test.
Line 1683 disables only FN_DSA_512, but Wallet gates on isAnyPqSchemeAllowed(). If another registered scheme such as ML_DSA_44 is enabled by default or a prior test, this no longer exercises the “no post-quantum scheme is activated” path.
Suggested fix
public void testApprovedListPqNotActivated() throws Exception {
chainBaseManager.getDynamicPropertiesStore().saveAllowFnDsa512(0L);
+ chainBaseManager.getDynamicPropertiesStore().saveAllowMlDsa44(0L);
FNDSA512 kp = new FNDSA512();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/src/test/java/org/tron/core/WalletTest.java` around lines 1682 -
1683, The test method testApprovedListPqNotActivated() currently only disables
the FN_DSA_512 scheme via saveAllowFnDsa512(0L), but since Wallet gates on
isAnyPqSchemeAllowed(), other enabled schemes like ML_DSA_44 will cause the test
to not properly exercise the "no post-quantum scheme activated" path. Disable
all registered post-quantum schemes in this test by calling the appropriate save
methods on chainBaseManager.getDynamicPropertiesStore() for each PQ scheme (not
just FN_DSA_512) to ensure no post-quantum scheme is enabled before testing the
inactive gate scenario.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
PQ multisig support added pq_auth_sig but several signature-count checks kept summing ECDSA signatures only: - consumeMultiSignFee only checked getSignatureCount(), so PQ-only and hybrid ECDSA+PQ multisig transactions escaped the multisign fee. - isSameSig (used by getVerifyTxs to skip re-verifying transactions already verified in the pending pool) compared only the ECDSA signature list, so a block transaction with a substituted pq_auth_sig but the same txId (txId hashes raw_data only) could be wrongly marked verified and skip real signature validation. - Wallet.getTransactionApprovedList built the approved list from ECDSA signatures only, missing the pq_auth_sig branch already present in its sibling TransactionUtil.getTransactionSignWeight, so PQ signers never showed up in the approval status. Add unit tests covering PQ-only, hybrid, and regression cases for all three paths. Also wraps two pre-existing >100-char lines in ProposalUtilTest to keep checkstyleTest passing.
bladehan1
force-pushed
the
feature/pq_multifee
branch
from
f0431d9 to
9dcb21f
Compare
bladehan1
changed the title
Collapse the three getVerifyTxs pq_auth_sig tests onto one shared helper instead of repeating the same transaction/pending-pool/ try-finally scaffolding in each. testApprovedListPqNotActivated only disabled FN_DSA_512, but Wallet gates on isAnyPqSchemeAllowed() across every registered scheme. Disable ML_DSA_44 too and assert isAnyPqSchemeAllowed() is false before building the transaction, so the test still exercises the intended path if another scheme is enabled by default or by a prior test.
Mark isSameSig package-private with @VisibleForTesting (precedented in this codebase by TransactionCapsule.logSlowSigVerify and several other @VisibleForTesting members) instead of exercising it only indirectly through getVerifyTxs's pending-pool/BlockCapsule scaffolding. Add 5 direct isSameSig tests covering null, pq_auth_sig content/count mismatch, identical pq_auth_sig, and the exact bug scenario (matching ECDSA list masking a differing pq_auth_sig list). Keep one getVerifyTxs-level integration test (getVerifyTxsClosesPqAuthSigBypass) as proof the fix is actually wired into the real bypass path, since a unit test of isSameSig alone can't prove that.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do
Fixes three places where PQ multisig (
pq_auth_sig) was not accounted for alongside ECDSA signatures, even though the existing permission/weight validation already treats the two as contributing independently to the same threshold:Manager.consumeMultiSignFeeonly checkedgetSignatureCount() > 1, so PQ-only and hybrid ECDSA+PQ multisig transactions escaped the multisign fee entirely.Manager.isSameSig(used bygetVerifyTxsto skip re-verifying transactions already verified in the pending pool) compared only the ECDSA signature list. SincetxIdhashesraw_dataonly, a block transaction with a substitutedpq_auth_sigbut the sametxIdcould be wrongly markedverified=trueand skip real signature validation inprocessTransaction— a signature-verification bypass.Wallet.getTransactionApprovedListbuilt the approved list from ECDSA signatures only, missing thepq_auth_sigbranch already present in its siblingTransactionUtil.getTransactionSignWeight, so PQ signers never showed up in the approval status response.Why required
These are correctness/security gaps introduced when PQ multisig support (
pq_auth_sig) was added: the fee, re-verification optimization, and approval-status reporting paths weren't updated to match the combined ECDSA+PQ signature semantics used everywhere else (e.g.TransactionCapsule.validateSignature,TransactionUtil.getTransactionSignWeight).Tests
ManagerTest:consumeMultiSignFeePqOnlyCharged,consumeMultiSignFeeHybridCharged,consumeMultiSignFeeSingleEcdsaNotCharged,consumeMultiSignFeeSinglePqNotCharged,getVerifyTxsPqAuthSigContentDifferRequiresReverify,getVerifyTxsPqAuthSigCountDifferRequiresReverify,getVerifyTxsPqAuthSigIdenticalSkipsReverifyWalletTest:testApprovedListPqOnlySigner,testApprovedListHybridEcdsaAndPq,testApprovedListPqNotActivatedManagerTest42/42,WalletTest51/51 with 1 pre-existing unrelated skip,ProposalUtilTest5/5).Follow up
None.
Extra details
Also wraps two pre-existing >100-character lines in
ProposalUtilTest(unrelated to this fix) to keepcheckstyleTestpassing.Summary by cubic
Count
pq_auth_sigwith ECDSA for multisign fee, re-verify checks, and approved lists. Fixes a verification bypass and makes PQ-only and hybrid multisig work as expected.Bug Fixes
Manager.consumeMultiSignFee.Manager.isSameSigalso checks PQ signature count and content to avoid skipping verification whentxIdmatches butpq_auth_sigdiffers.Wallet.getTransactionApprovedListmerges ECDSA and PQ signers and rejectspq_auth_sigwhen no PQ scheme is activated.Refactors
Manager.isSameSig@VisibleForTestingand added focused unit tests; consolidatedgetVerifyTxsPQ cases into a helper and hardened the inactive-PQ test by disabling all PQ schemes and asserting the global gate.Written for commit 9094a1b. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests