feat(nano): Add reentrancy protection by default#1335
Conversation
|
| Branch | feat/nano-reentrancy-protection |
| Testbed | ubuntu-22.04 |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result minutes (m) (Result Δ%) | Lower Boundary minutes (m) (Limit %) | Upper Boundary minutes (m) (Limit %) |
|---|---|---|---|---|
| sync-v2 (up to 20000 blocks) | 📈 view plot 🚷 view threshold | 1.82 m(+9.75%)Baseline: 1.66 m | 1.49 m (82.00%) | 1.82 m (99.77%) |
050ea49 to
c8dd71c
Compare
02564c6 to
4e6f979
Compare
| if allow_reentrancy: | ||
| return | ||
|
|
||
| for call_record in self._call_info.stack: |
There was a problem hiding this comment.
The current code runs in O(n) time, where n is the maximum allowed depth (currently set to 100). We could optimize it to run in O(1) time by maintaining counters in a dict[tuple[ContractId, method_name], int]. Is this optimization worth implementing? Should we reduce the maximum allowed depth?
There was a problem hiding this comment.
100 equality comparisons is too cheap, I don't think it's a problem.
There was a problem hiding this comment.
It can get up to 1+2+3+...+100 = 100*101/2 = 5050 comparisons, actually. But I guess it's still not much and shouldn't be a problem. We can always optimize it later. Maybe just leave a note about it?
There was a problem hiding this comment.
@jansegre What do you think? This check runs in O(n) where n is the stack size. So, the overall algorithm runs in O(n^2) where n is the maximum stack size it reaches during execution. is it worth the effort to reduce the time complexity to O(n)? Any DoS attack vectors here?
There was a problem hiding this comment.
I feel like this can be optimized, but I don't think it should be a blocker. Do we have any idea about the order of magnitude of time it takes when n=100? Being proportional to ~10.000 times a single verification should be manageable I think, but certainly not ideal.
There was a problem hiding this comment.
To be clear, I don't think this is a blocker to merge this PR.
I think it's OK to add a task to optimize it later. I just think it's possible to avoid looking at the whole stack every time, maybe memoizing the last call on the same stack somehow so each check doesn't need to go up the stack again, only up to the last call.
4e6f979 to
031fc41
Compare
10536dc to
89ac14b
Compare
89ac14b to
7c572c3
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1335 +/- ##
==========================================
- Coverage 85.66% 85.61% -0.06%
==========================================
Files 424 424
Lines 32120 32131 +11
Branches 4997 5001 +4
==========================================
- Hits 27516 27508 -8
- Misses 3604 3615 +11
- Partials 1000 1008 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Add a test to cover the behavior change from my thread (the reenter example), that is, cover that reentrancy is prevented even if the called method is not the same.
7c572c3 to
387a74f
Compare
387a74f to
dc6b1d7
Compare
dc6b1d7 to
93ded98
Compare
Motivation
Add a reentrancy protection enabled by default at the method level.
Acceptance Criteria
allow_reentrancy: boolto@public(...)decorator._validate_reentrancy()to runner.Checklist
master, confirm this code is production-ready and can be included in future releases as soon as it gets merged