Repository navigation
Conversation
|
|
benjamin-stacks
left a comment
There was a problem hiding this comment.
Thank you so much for the contribution, I think we should absolutely pull this in!
I do however have concerns about putting the table in a global singleton, and about the possibilities of dependencies for the regexes. See my inline comment.
Do you think you might be able to address that?
benjamin-stacks
left a comment
There was a problem hiding this comment.
Nice work, thank you! Just one small nit.
Adding @federico-stacks as a second reviewer.
| /// The read-only RPC memory limit must be threaded from `ConnectionOptions` | ||
| /// into the HTTP server state so the handlers can set the correct budget. | ||
| #[test] | ||
| fn test_read_only_call_max_mem_bytes_threaded_into_http() { |
There was a problem hiding this comment.
While I don't like that we're removing this test without replacement, I understand that it's tricky to find a good way to test this now. I think it's justifiable.
federico-stacks
left a comment
There was a problem hiding this comment.
LGTM! I've just added one nit.
A couple of other things to flag:
- The changelog entry for this change is missing. You can add one following these guidelines.
- There are a few merge conflicts that need to be resolved.
# Conflicts: # stackslib/src/net/httpcore.rs # stackslib/src/net/tests/httpcore.rs
There was a problem hiding this comment.
I noticed that some tests fail consistently on this PR (I re-ran them a bunch of times):
- nakamoto_integrations::check_as_contract_rollback
- nakamoto_integrations::check_block_time_keyword
- nakamoto_integrations::check_restrict_assets_rollback
- nakamoto_integrations:: check_with_stacking_allowances_delegate_stx
- nakamoto_integrations:: check_with_stacking_allowances_stack_stx
They pass on main, so this doesn’t appear to be a bug introduced by this change. More likely, the change exposes an existing race condition in these tests. The failures occur in next_block_and_mine_commit (timeout). My guess is that, before the patch, rebuilding the route table for each HTTP connection made the /v2/info RPC slow enough for the relayer to submit its block commit first.
The epoch 3.3 boot loop in these tests only waits for a Stacks block, then `next_block_and_mine_commit` immediately mines the next burn block. If the relayer hasn't submitted its commit for the new tenure yet, that burn block has an empty sortition, the miner extends its tenure without committing to a newer tip, and the helper times out. Faster RPC handling made this race lose consistently, so wait for a commit on the current burn tip first, as `wait_for_tip_commit` does for neon tests.
the race was on to mine the next burn block before the relayer’s commit arrived, fixed it by waiting for the naka tip commit before mining |
Description
each inbound http connection rebuilt the rpc route table, registering 52 handlers and compiling 156 regexes (~7 ms per p2p connection).
HttpPeernow builds the route table once fromConnectionOptionsand reuses it across connections.HttpRequest::verb,path_regex, andpath_regex_permissiveare associated functions withwhere Self: Sized, keeping the trait object-safe.RPCRouteTable::new(&ConnectionOptions)builds routes and handlers,HttpPeerowns the table and passes it toConversationHttp.StacksHttp::with_routesshares compiled routes viaArc<[RPCRoute]>while creating fresh handlers per connection.register_rpc_methodskeeps the same body and order, now onRPCRoutesBuilder.StacksHttp::new(addr, &conn_opts)remains for tests and one-off use.Performance
StacksHttp::newRPCRouteTable::newStacksHttp::with_routesGET /v2/infoGET /v2/infoon open connectionShared regexes retain their match caches, so new connections avoid the previous cold-start cost.
Tests
Applicable issues
Additional info (benefits, drawbacks, caveats)
HttpRequest::verb,path_regexandpath_regex_permissiveno longer take&self.ConversationHttp::newtakes&RPCRouteTable.StacksHttp::new_clientno longer takesconn_opts.StacksHttpno longer exposes handler configuration fields used only during construction.HttpPeerbuilds its table fromconnection_optsat construction. Changing those options afterwards does not update existing handlers.Checklist
docs/property-testing.md)changelog.d/README.md). If this PR breaks anything for node operators or users, or requires them to manually do anything (such as adjust a setting), use the breaking category.rpc/openapi.yamlfor RPC endpoints,event-dispatcher.mdfor new events)clarity-benchmarkingrepo