foundry
foundry copied to clipboard
foundry-rs
Forge test doesn't treat calls to external libraries like part of a parent transaction but as independent transactions
Component
Forge
Have you ensured that all of these are up to date?
- [X] Foundry
- [X] Foundryup
What version of Foundry are you on?
forge 0.2.0 (63fff35)
What command(s) is the bug in?
forge test
Operating System
macOS (Apple Silicon)
Describe the bug
running a test where library A uses library B in a negative path (expecting a revert) fails because Forge seems unaware of the fact that the calls from contract A to library B are part of a single transaction from EOA_X to library A. Therefore, if first step of the tx is library A calling library B and this call is successful, then the expect revert on the tx will flag the test as a failure because the tx didn't fail even though the call to the library A didn't get to the point of failure due to the expect revert stopping the test. This behavior was noticed to happen under very specific circumstances since calling the function that contains the failure line directly doesn't end in a failed test. It only happens if a function that later calls the other function with the failure line is called.
My specific example is this one:
library A{
using library_B for uint256;
function calculate_c512(uint256 Xn, uint256 Sn, uint256 DnL, uint256 DnH) internal pure returns (uint256 Cn) {
(uint numrL, uint numrH) = getNumeratorC512(Xn, Sn, DnL, DnH);
(uint numrWADL, uint numrWADH) = numrL.mul512x256(numrH, FixedPointMathLib.WAD);
Cn = (numrWADL).div512x256(numrWADH, Xn);
}
/// helper function
function getNumeratorC512(uint256 Xn, uint256 Sn, uint256 DnL, uint256 DnH) public pure returns (uint256 numrL, uint256 numrH) {
(uint256 xSqrL, uint256 xSqrH) = getXsqrHalvedC512(Xn);
(uint256 subsndNumL, uint256 subsndNumH) = getSubstrahendC512(xSqrL, xSqrH, Sn);
// this ensures that the result will not be negative
checkNumeratorPositiveC512(subsndNumL, subsndNumH, DnL, DnH);
(numrL, numrH) = DnL.sub512x512(DnH, subsndNumL, subsndNumH);
}
}
The line of failure is checkNumeratorPositiveC512(subsndNumL, subsndNumH, DnL, DnH); in getNumeratorC512.
If I run tests for each one of them, and they are practically identical except for the fact that I test in one of them calling the helper function directly and the other one calls the actual function, they should revert at exactly the same point since getNumeratorC512 is called in the first line of calculate_c512:
function testEquations_CofN512_CalculateNumeratorFuzz(uint256 Xn, uint256 Sn, uint256 DnL, uint256 DnH) public {
(Xn, Sn, DnL, DnH) = boundInputs(Xn, Sn, DnL, DnH);
(uint256 xSqrL, uint256 xSqrH) = Xn.getXsqrHalvedC512();
(uint256 subsndNumL, uint256 subsndNumH) = xSqrL.getSubstrahendC512(xSqrH, Sn);
// this ensures that the result will not be negative
bool willRevert = DnH < subsndNumH || (DnH == subsndNumH && DnL < subsndNumL);
if (willRevert) vm.expectRevert(abi.encodeWithSignature("ResultBelowPMin()")); /// <- passes
(uint256 numrL, uint256 numrH) = Xn.getNumeratorC512(Sn, DnL, DnH);
}
function testEquations_CofN512_CalculateCFuzz(uint256 Xn, uint256 Sn, uint256 DnL, uint256 DnH) public {
(Xn, Sn, DnL, DnH) = boundInputs(Xn, Sn, DnL, DnH);
(uint256 xSqrL, uint256 xSqrH) = Xn.getXsqrHalvedC512();
(uint256 subsndNumL, uint256 subsndNumH) = xSqrL.getSubstrahendC512(xSqrH, Sn);
// this ensures that the result will not be negative
bool willRevert = DnH < subsndNumH || (DnH == subsndNumH && DnL < subsndNumL);
if (willRevert) vm.expectRevert(abi.encodeWithSignature("ResultBelowPMin()")); /// <- fails
uint solVal = Xn.calculate_c512(Sn, DnL, DnH);
}
where the first one reverts as expected and therefore the test is successful, but where the second one fails because the transaction didn't revert as expected.
Logs from testEquations_CofN512_CalculateNumeratorFuzz test (this is the one that is Ok):
[PASS] testEquations_CofN512_CalculateNumeratorFuzz(uint256,uint256,uint256,uint256) (runs: 8215, μ: 24948, ~: 21806)
Traces:
[22610] CofN512FuzzTests::testEquations_CofN512_CalculateNumeratorFuzz(9159, 338, 92427370533738632727432635211694201858019100662723587849922113037932888071 [9.242e73], 392)
├─ [0] console::log("Bound Result", 99999999999999999999999000009160 [9.999e31]) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("Bound Result", 1329227995784915872903807060280343914 [1.329e36]) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("Bound Result", 7991) [staticcall]
│ └─ ← [Stop]
├─ [3806] MetroidEquations::getXsqrHalvedC512(99999999999999999999999000009160 [9.999e31]) [delegatecall]
│ ├─ [414] MathLibraryAbstractionLayer::mul256x256(99999999999999999999999000009160 [9.999e31], 99999999999999999999999000009160 [9.999e31]) [delegatecall]
│ │ └─ ← [Return] 9999999999999999999999800001832000000000000000999981680083905600 [9.999e63], 0
│ └─ ← [Return] 4999999999999999999999900000916000000000000000499990840041952800 [4.999e63], 0
├─ [1299] MetroidEquations::getSubstrahendC512(4999999999999999999999900000916000000000000000499990840041952800 [4.999e63], 0, 1329227995784915872903807060280343914 [1.329e36]) [delegatecall]
│ ├─ [505] MathLibraryAbstractionLayer::mul512x256(4999999999999999999999900000916000000000000000499990840041952800 [4.999e63], 0, 1329227995784915872903807060280343914 [1.329e36]) [delegatecall]
│ │ └─ ← [Return] 94462997441653099451176459878827809087094626915415700104600968985925209901376 [9.446e76], 57397185098744507225034 [5.739e22]
│ └─ ← [Return] 94462997441653099451176459878827809087094626915415700104600968985925209901376 [9.446e76], 57397185098744507225034 [5.739e22]
├─ [0] VM::expectRevert(ResultBelowPMin())
│ └─ ← [Return]
├─ [2532] MetroidEquations::getNumeratorC512(99999999999999999999999000009160 [9.999e31], 1329227995784915872903807060280343914 [1.329e36], 7991, 0) [delegatecall]
│ ├─ [414] MathLibraryAbstractionLayer::mul256x256(99999999999999999999999000009160 [9.999e31], 99999999999999999999999000009160 [9.999e31]) [delegatecall]
│ │ └─ ← [Return] 9999999999999999999999800001832000000000000000999981680083905600 [9.999e63], 0
│ ├─ [505] MathLibraryAbstractionLayer::mul512x256(4999999999999999999999900000916000000000000000499990840041952800 [4.999e63], 0, 1329227995784915872903807060280343914 [1.329e36]) [delegatecall]
│ │ └─ ← [Return] 94462997441653099451176459878827809087094626915415700104600968985925209901376 [9.446e76], 57397185098744507225034 [5.739e22]
│ └─ ← [Revert] ResultBelowPMin()
└─ ← [Stop]
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.59s (4.59s CPU time)
Logs from testEquations_CofN512_CalculateCFuzz test (this is the one that fails):
Traces:
[18890] CofN512FuzzTests::testEquations_CofN512_CalculateCFuzz(0, 0, 0, 0)
├─ [0] console::log("Bound Result", 1000000000 [1e9]) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("Bound Result", 1000) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("Bound Result", 0) [staticcall]
│ └─ ← [Stop]
├─ [3806] MetroidEquations::getXsqrHalvedC512(1000000000 [1e9]) [delegatecall]
│ ├─ [414] MathLibraryAbstractionLayer::mul256x256(1000000000 [1e9], 1000000000 [1e9]) [delegatecall]
│ │ └─ ← [Return] 1000000000000000000 [1e18], 0
│ └─ ← [Return] 500000000000000000 [5e17], 0
├─ [1299] MetroidEquations::getSubstrahendC512(500000000000000000 [5e17], 0, 1000) [delegatecall]
│ ├─ [505] MathLibraryAbstractionLayer::mul512x256(500000000000000000 [5e17], 0, 1000) [delegatecall]
│ │ └─ ← [Return] 500000000000000000000 [5e20], 0
│ └─ ← [Return] 500000000000000000000 [5e20], 0
├─ [0] VM::expectRevert(ResultBelowPMin())
│ └─ ← [Return]
├─ [414] MathLibraryAbstractionLayer::mul256x256(1000000000 [1e9], 1000000000 [1e9]) [delegatecall]
│ └─ ← [Return] 1000000000000000000 [1e18], 0
└─ ← [Revert] call did not revert as expected
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 26.02ms (21.58ms CPU time)
Notice how in the first logs it is clear that the calls to MathLibraryAbstractionLayer come from MetroidEquations::getNumeratorC512 whereas the second logs show that calls to MetroidEquations::getNumeratorC512 are mistakenly not part of the MetroidEquations::calculate_c512 (this function call doesn't show up at all in the logs actually) which is exposing the actual bug being reported here.
EDIT
It was stated that A was a contract, but it is in reality a library. The issue happens when library_A calls library_B in a test and Forge seems to be unaware that the calls to library_B from library_A are part of a single transaction when the function called is a second-layer function in the library.
expectRevert expects the next frame to revert. delegatecalls to a library is a frame as well
In your case getNumeratorC512 is a public function thus it is getting dispatched as a delegatecall and expectRevert works correctly.
However, calculate_c512 is an internal function which is getting inlined into a contract and no delegatecall is performed when it invoked (which can be seen in traces), thus the first frame seen by expectRevert is invocation of mul512x256
This is interesting though as I would expect invocation of getNumeratorC512 in calculate_c512 to perform a delegatecall, but it seems that if public library function is invoked in internal function of the same library, it will cause public fn to get inlined as well, which is weird as it would cause duplicating bytecode being deployed in both library and contract using it
Hm. That's a good find because I had forgotten about the mixed visibilities I had there. However, the only reason I started to make the other functions public was because the issue was already there when there was only 1 function calculate_c512, and I was not able to spot the problem thinking the error was in my code, so I made the other helper functions and made the library a regular contract at some point. Anyways, long story short, I tried making all the functions in library_A internal to test your point, so now getNumeratorC512 and calculate_c512 are both internal. I still have the same issue.
sure!
Traces:
[14421] CofN512FuzzTests::testEquations_CofN512_CalculateNumeratorFuzz(4136799322481705091955576 [4.136e24], 134193314344108 [1.341e14], 100000000000000000 [1e17], 0)
├─ [0] console::log("Bound Result", 4136799322481705091955576 [4.136e24]) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("Bound Result", 134193314344108 [1.341e14]) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("Bound Result", 100000000000000000 [1e17]) [staticcall]
│ └─ ← [Stop]
├─ [392] MathLibraryAbstractionLayer::mul256x256(4136799322481705091955576 [4.136e24], 4136799322481705091955576 [4.136e24]) [delegatecall]
│ └─ ← [Return] 17113108634485094279843588697454873929987957491776 [1.711e49], 0
├─ [505] MathLibraryAbstractionLayer::mul512x256(8556554317242547139921794348727436964993978745888 [8.556e48], 0, 134193314344108 [1.341e14]) [delegatecall]
│ └─ ← [Return] 1148232383196163535505025695706415258810544877106422631122027904 [1.148e63], 0
├─ [0] VM::expectRevert(ResultBelowPMin())
│ └─ ← [Return]
├─ [392] MathLibraryAbstractionLayer::mul256x256(4136799322481705091955576 [4.136e24], 4136799322481705091955576 [4.136e24]) [delegatecall]
│ └─ ← [Return] 17113108634485094279843588697454873929987957491776 [1.711e49], 0
└─ ← [Revert] call did not revert as expected
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 2.82s (2.81s CPU time)
This is calling getNumeratorC512
Traces:
[14456] CofN512FuzzTests::testEquations_CofN512_CalculateCFuzz(4136799322481705091955576 [4.136e24], 134193314344108 [1.341e14], 100000000000000000 [1e17], 0)
├─ [0] console::log("Bound Result", 4136799322481705091955576 [4.136e24]) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("Bound Result", 134193314344108 [1.341e14]) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("Bound Result", 100000000000000000 [1e17]) [staticcall]
│ └─ ← [Stop]
├─ [392] MathLibraryAbstractionLayer::mul256x256(4136799322481705091955576 [4.136e24], 4136799322481705091955576 [4.136e24]) [delegatecall]
│ └─ ← [Return] 17113108634485094279843588697454873929987957491776 [1.711e49], 0
├─ [505] MathLibraryAbstractionLayer::mul512x256(8556554317242547139921794348727436964993978745888 [8.556e48], 0, 134193314344108 [1.341e14]) [delegatecall]
│ └─ ← [Return] 1148232383196163535505025695706415258810544877106422631122027904 [1.148e63], 0
├─ [0] VM::expectRevert(ResultBelowPMin())
│ └─ ← [Return]
├─ [392] MathLibraryAbstractionLayer::mul256x256(4136799322481705091955576 [4.136e24], 4136799322481705091955576 [4.136e24]) [delegatecall]
│ └─ ← [Return] 17113108634485094279843588697454873929987957491776 [1.711e49], 0
└─ ← [Revert] call did not revert as expected
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 2.83s (2.82s CPU time)
This is calling calculate_c512.
So it looks like you are into something here because now both calls fail. This is still not the desired behavior, but the function visibility definitely was causing one to fail the test and the other to pass. At least now both fail the test.
ah so right now when both functions are internal they are both getting inlined, thus first frame created with invocation of those fn is a MathLibraryAbstractionLayer delegatecall which does not revert
to test this you can either make both functions public so that each of them creates a new frame when invoked
also you can create a wrapper around them for testing or just a helper function on a test contract
for example
function calculate_c512(uint256 Xn, uint256 Sn, uint256 DnL, uint256 DnH) internal pure returns (uint256 Cn) {
Xn.getNumeratorC512(Sn, DnL, DnH);
}
function test(....) public {
vm.expectRevert(...);
this.calculate_c512(...)
}
Yeah that would totally work. I do think Forge test should be smart enough to handle this tho.
unfortunately after compiler optimizations it is really hard to tell whether certain bytecode corresponds to an internal library, handling this is non-trivial just as handling reverts in internal contract functions for example
Ok I see. Thank you for your time @klkvr. This definitely helped me understand my issue even though it seems to be now in my hands to handle this kind of situations.
