Skip to content

fix(evm): preserve gas cost in cyclic chunks #305

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

Draft
abmcar wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix(evm): preserve gas cost in cyclic chunks #305

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
93 changes: 89 additions & 4 deletions src/evm/evm_cache.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,75 @@ static bool bitsetTest(const std::vector<uint64_t> &Bits, size_t Index) {
return (Bits[Index / 64] & (uint64_t{1} << (Index % 64))) != 0;
}

static std::vector<uint8_t>
computeInCycle(const std::vector<GasBlock> &Blocks) {
const size_t NumBlocks = Blocks.size();
std::vector<int32_t> Index(NumBlocks, -1);
std::vector<int32_t> Lowlink(NumBlocks, 0);
std::vector<uint8_t> OnStack(NumBlocks, 0);
std::vector<uint32_t> Stack;
std::vector<uint8_t> InCycle(NumBlocks, 0);
int32_t CurIndex = 0;

std::function<void(uint32_t)> StrongConnect = [&](uint32_t Node) {
Index[Node] = CurIndex;
Lowlink[Node] = CurIndex;
++CurIndex;
Stack.push_back(Node);
OnStack[Node] = 1;

for (uint32_t Succ : Blocks[Node].Succs) {
if (Succ >= NumBlocks) {
continue;
}
if (Index[Succ] < 0) {
StrongConnect(Succ);
Lowlink[Node] = std::min(Lowlink[Node], Lowlink[Succ]);
} else if (OnStack[Succ] != 0) {
Lowlink[Node] = std::min(Lowlink[Node], Index[Succ]);
}
}

if (Lowlink[Node] != Index[Node]) {
return;
}

std::vector<uint32_t> Scc;
while (!Stack.empty()) {
const uint32_t Top = Stack.back();
Stack.pop_back();
OnStack[Top] = 0;
Scc.push_back(Top);
if (Top == Node) {
break;
}
}

if (Scc.size() > 1) {
for (uint32_t Id : Scc) {
InCycle[Id] = 1;
}
return;
}

const uint32_t Only = Scc.front();
for (uint32_t Succ : Blocks[Only].Succs) {
if (Succ == Only) {
InCycle[Only] = 1;
break;
}
Comment on lines +378 to +424
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

The recursive lambda StrongConnect can cause stack overflow for deeply nested or large graphs due to unbounded recursion depth. For EVM bytecode with complex control flow graphs containing thousands of blocks, this could be problematic.

Consider converting this to an iterative implementation using an explicit stack to avoid potential stack overflow issues, or document the assumption about maximum CFG depth if that's acceptable for this use case.

Suggested change
std::function<void(uint32_t)> StrongConnect = [&](uint32_t Node) {
Index[Node] = CurIndex;
Lowlink[Node] = CurIndex;
++CurIndex;
Stack.push_back(Node);
OnStack[Node] = 1;
for (uint32_t Succ : Blocks[Node].Succs) {
if (Succ >= NumBlocks) {
continue;
}
if (Index[Succ] < 0) {
StrongConnect(Succ);
Lowlink[Node] = std::min(Lowlink[Node], Lowlink[Succ]);
} else if (OnStack[Succ] != 0) {
Lowlink[Node] = std::min(Lowlink[Node], Index[Succ]);
}
}
if (Lowlink[Node] != Index[Node]) {
return;
}
std::vector<uint32_t> Scc;
while (!Stack.empty()) {
const uint32_t Top = Stack.back();
Stack.pop_back();
OnStack[Top] = 0;
Scc.push_back(Top);
if (Top == Node) {
break;
}
}
if (Scc.size() > 1) {
for (uint32_t Id : Scc) {
InCycle[Id] = 1;
}
return;
}
const uint32_t Only = Scc.front();
for (uint32_t Succ : Blocks[Only].Succs) {
if (Succ == Only) {
InCycle[Only] = 1;
break;
}
std::function<void(uint32_t)> StrongConnect = [&](uint32_t StartNode) {
// Iterative version of Tarjan's algorithm to avoid recursive calls.
struct Frame {
uint32_t Node;
size_t SuccIndex;
};
std::vector<Frame> DfsStack;
DfsStack.push_back(Frame{StartNode, 0});
while (!DfsStack.empty()) {
Frame &CurFrame = DfsStack.back();
const uint32_t Node = CurFrame.Node;
if (Index[Node] < 0) {
Index[Node] = CurIndex;
Lowlink[Node] = CurIndex;
++CurIndex;
Stack.push_back(Node);
OnStack[Node] = 1;
}
const auto &Succs = Blocks[Node].Succs;
bool Descended = false;
while (CurFrame.SuccIndex < Succs.size()) {
const uint32_t Succ = Succs[CurFrame.SuccIndex];
++CurFrame.SuccIndex;
if (Succ >= NumBlocks) {
continue;
}
if (Index[Succ] < 0) {
// Tree edge: descend into successor.
DfsStack.push_back(Frame{Succ, 0});
Descended = true;
break;
}
if (OnStack[Succ] != 0) {
// Successor in current SCC stack: update lowlink with its index.
Lowlink[Node] = std::min(Lowlink[Node], Index[Succ]);
}
}
if (Descended) {
continue;
}
// All successors processed; check if Node is a root of an SCC.
if (Lowlink[Node] == Index[Node]) {
std::vector<uint32_t> Scc;
while (!Stack.empty()) {
const uint32_t Top = Stack.back();
Stack.pop_back();
OnStack[Top] = 0;
Scc.push_back(Top);
if (Top == Node) {
break;
}
}
if (Scc.size() > 1) {
for (uint32_t Id : Scc) {
InCycle[Id] = 1;
}
} else if (!Scc.empty()) {
const uint32_t Only = Scc.front();
for (uint32_t Succ : Blocks[Only].Succs) {
if (Succ == Only) {
InCycle[Only] = 1;
break;
}
}
}
}
// Propagate lowlink information to the parent in the DFS stack.
DfsStack.pop_back();
if (!DfsStack.empty()) {
const uint32_t ParentNode = DfsStack.back().Node;
Lowlink[ParentNode] = std::min(Lowlink[ParentNode], Lowlink[Node]);
}

Copilot uses AI. Check for mistakes.
}
};

for (uint32_t Node = 0; Node < NumBlocks; ++Node) {
if (Index[Node] < 0) {
StrongConnect(Node);
}
}

return InCycle;
}

static bool bitsetEqual(const std::vector<uint64_t> &A,
const std::vector<uint64_t> &B) {
return A == B;
Expand Down Expand Up @@ -871,6 +940,7 @@ static bool buildGasChunksSPP(const zen::common::Byte *Code, size_t CodeSize,
for (size_t Index = 0; Index < RevTopo.size(); ++Index) {
RevTopoIndex[RevTopo[Index]] = Index;
}
const std::vector<uint8_t> InCycle = computeInCycle(Blocks);

std::vector<LoopInfo> Loops;
std::vector<int32_t> LoopOf;
Expand All @@ -885,9 +955,19 @@ static bool buildGasChunksSPP(const zen::common::Byte *Code, size_t CodeSize,
Metering[Id] = Blocks[Id].Cost;
}

std::vector<uint64_t> NonCycleMask(bitsetWordCount(Blocks.size()), 0);
for (size_t Id = 0; Id < Blocks.size(); ++Id) {
if (Id < InCycle.size() && InCycle[Id] == 0) {
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

The redundant bounds check Id < InCycle.size() adds unnecessary complexity. Since InCycle is always sized to Blocks.size() (from line 370, 375), and the loop iterates from 0 to Blocks.size(), this check will always be true. This check is redundant and should be removed for clarity.

Copilot uses AI. Check for mistakes.
bitsetSet(NonCycleMask, Id);
}
}

if (!UseLinearSPP) {
for (uint32_t NodeId : RevTopo) {
lemma614Update(NodeId, Blocks, &BackEdges, nullptr, Metering);
if (NodeId < InCycle.size() && InCycle[NodeId] != 0) {
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

The bounds check NodeId < InCycle.size() is redundant because InCycle is created with size NumBlocks (line 375), and NodeId comes from iterating over RevTopo which contains node indices less than NumBlocks. The same InCycle vector is used throughout without any size changes.

This check pattern appears consistently but unnecessarily at lines 967, 999, and 1010. While not incorrect, it adds unnecessary overhead and code complexity. Consider removing these redundant checks or add a comment explaining why they are needed if there's a specific edge case they guard against.

Suggested change
if (NodeId < InCycle.size() && InCycle[NodeId] != 0) {
if (InCycle[NodeId] != 0) {

Copilot uses AI. Check for mistakes.
continue;
}
lemma614Update(NodeId, Blocks, &BackEdges, &NonCycleMask, Metering);
Comment on lines 965 to +970
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

The code now uses two different mechanisms to identify loops/cycles: (1) dominator-based back edges via BackEdges, and (2) SCC-based cycle detection via InCycle. These serve different purposes but may produce conflicting results.

Specifically, BackEdges identifies loop back edges (edges where target dominates source), while InCycle identifies all nodes in any strongly connected component. A node can be in a cycle without having any back edges (e.g., in an irreducible loop), and a node with back edges might not be in the SCC identified as a cycle.

This dual approach could lead to inconsistent treatment of loop nodes. Consider documenting why both mechanisms are needed and how they interact, or simplifying to use a single, consistent approach to loop identification.

Copilot uses AI. Check for mistakes.
}
} else {
std::vector<std::vector<uint32_t>> Recorded(Loops.size());
Expand Down Expand Up @@ -916,16 +996,21 @@ static bool buildGasChunksSPP(const zen::common::Byte *Code, size_t CodeSize,
return RevTopoIndex[A] < RevTopoIndex[B];
});
for (uint32_t NodeId : Order) {
lemma614Update(NodeId, Blocks, nullptr, &Loops[LoopId].NodeMask,
Metering);
if (NodeId < InCycle.size() && InCycle[NodeId] != 0) {
continue;
}
lemma614Update(NodeId, Blocks, nullptr, &NonCycleMask, Metering);
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

The AllowedMask parameter has been changed from &Loops[LoopId].NodeMask to &NonCycleMask, which fundamentally alters the algorithm's behavior.

The original code restricted updates to successors within the specific loop being processed (via Loops[LoopId].NodeMask). The new code only restricts to non-cyclic successors globally (via NonCycleMask), which means it will incorrectly consider successors outside the current loop during loop-specific processing.

For example, if a non-cyclic node N in loop L1 has a successor S that is non-cyclic and in loop L2 (or not in any loop), the update will now incorrectly include S in the minimum cost calculation. This breaks the loop-local invariant that the SPP algorithm depends on.

The correct approach would be to compute an intersection of both masks, or to restructure the logic to maintain the loop-specific constraint while also excluding cyclic nodes.

Copilot uses AI. Check for mistakes.
}
LoopProcessed[LoopId] = 1;
};

for (uint32_t NodeId : RevTopo) {
const int32_t LoopId = (NodeId < LoopOf.size()) ? LoopOf[NodeId] : -1;
if (LoopId < 0) {
lemma614Update(NodeId, Blocks, &BackEdges, nullptr, Metering);
if (NodeId < InCycle.size() && InCycle[NodeId] != 0) {
continue;
}
lemma614Update(NodeId, Blocks, &BackEdges, &NonCycleMask, Metering);
} else {
Recorded[LoopId].push_back(NodeId);
++RecordedCount[LoopId];
Expand Down
Loading