Repository: stacks-network/stacks-core (language: python)

Issue:
[Testing] Extract tests in `stackslib/src/net/server.rs` and run `#[ignore]`'d ones in CI
See title
Feat: Shadow block recovery mechanism
This introduces the notion of "shadow blocks" and "shadow tenures" as a means of recovering from a chain stall.  In the event of an irrecoverable chain stall, we'd pass an emergency SIP to declare that one or more empty sortitions actually commit to a "shadow block" that contains whatever state is needed to get the chain un-stuck.  All nodes would ship with copies of these shadow blocks, and would process them the same as any other kind of block (except that they could not be mined, uploaded, or downloaded).

Leaving as a draft for now until I can expand test coverage.  This touches a lot of consensus-critical code paths.

----
Here is a summary of consensus changes for shadow blocks:
* All shadow blocks _MUST_ have a `version` whose highest bit is set.
* If any block in a tenure is a shadow block, then all blocks in that tenure _MUST_ be shadow blocks (it's a "shadow tenure")
* No non-shadow blocks can be inserted into a shadow tenure.
* The block-commit of the first child tenure of a shadow block tenure _MUST_ set `parent_vtxindex` to 0, which is the burnchain coinbase transaction.  We assume that shadow blocks all descend from their respective PoX anchor blocks, so this will _not_ be checked for the first child tenure.  Instead, the block's parent sortition will be checked to verify that it is a shadow tenure and that the block-commit `parent_vtxindex` is 0 when validating the child block.
* The VRF proof of a shadow block will not be validated (since there's no seed to use)
* The miner signature of a shadow block will not be validated (since there's no miner)
* For the purposes of finding the PoX anchor block, a shadow tenure in the prepare-phase always counts as a sortition, even if there were no block-commits in that burnchain block.
* The signing weight of a shadow block is the maximal weight of the reward set.  It's as if all signers signed it.
* The coinbase of a shadow block _MUST_ be the burn address for the network.  Shadow blocks will produce STX, but no one will receive it.
* The memo field of the coinbase of a shadow block _MUST_ be the hash of the tenure-start block of the shadow block's parent tenure.  This is required because shadow blocks do not have corresponding block-commits.  Instead, we capture the tenure start block hash that would have gone into the block-commit as the memo field in the shadow block coinbase.
* Shadow blocks do not increase the total BTC spend for the chain
* The VRF proof for the child of a shadow block must come from the VRF seed of the shadow block -- i.e. the hash of the VRF proof in the parent shadow block's tenure's coinbase.

Feat/rpc endpoints to fetch data from key
### Description

Add two new endpoints:
- `/v2/clarity_marf_value/:clarity_marf_key`
- `/v2/clarity_metadata/:principal/:contract_name/:clarity_metadata_key`

I'm currently working on a Clarinet feature that allows to simulate running (or, said differently, to fork the mainnet state in the simnet data store). This requires the ability to fetch values from marf and metadata keys.

These new endpoints are similar to already existing endpoint (such as `getdatavar`,  `getmapentry`,  `getcontractsrc`, etc).

### Applicable issues

N/A

### Additional info (benefits, drawbacks, caveats)

Read more about this feature in the Clarinet issue: https://github.com/hirosystems/clarinet/issues/1503

I confirmed that the 2 new endpoints allow to achieve the desired goal by running a local devnet with these changes and forking the Clarinet simnet state from it. 

### Checklist

- [x] Test coverage for new or modified code paths
- [ ] Changelog is updated
- [ ] Required documentation changes (e.g., `docs/rpc/openapi.yaml` and `rpc-endpoints.md` for v2 endpoints, `event-dispatcher.md` for new events)

[Testing] Extract tests in `stackslib/src/net/server.rs` and run `#[ignore]`'d ones in CI
See title


Discussion / hints:

N.B. This PR needs to be followed by (or extend with) a PR to re-mine a sequence of blocks as shadow blocks, so we can fix a consensus bug on testnet.

We'll need more elaborate chain replay logic in the future to preserve causal relationships between affected transactions if possible.  At a minimum today, we can preserve the VRF proofs.
Okay, this PR is getting kinda big.  I'm going to hold off on adding the tooling for generating shadow blocks for the time being.  I'll add that as a separate PR so we can move this one along.
@kantai @jferrant @obycode This is ready for re-review.  I'm going to work on tooling for (quickly) synthesizing shadow blocks via `stacks-inspect` so we can test this on the Nakamoto testnet.
Look at that.

All of that.

All those green :heavy_check_mark:'s in CI.

Outstanding work on making them all reliable @jferrant!  It drastically improves our confidence that we can make consensus-critical changes like this.  Well done!
Okay, there's definitely some latent bug here in the test harness that's causing a failure.  I think it may be the same one discovered by @hugocaillard in https://github.com/stacks-network/stacks-core/pull/4997.
I went ahead and made the requisite changes to the MARF, `ClarityBackingStore` and all its implementations, and `ClarityDB` so you can load values by `TrieHash`.  I also removed `TriePath` from the codebase, since it's used exactly the same way that `TrieHash` is.

My comments on the getter for Clarity metadata still stand, however.  If we're going to take metadata keys as input, we need to validate them and return HTTP 400 if they're semantically invalid.
The reason we *didn't* do that is because that invokes the allocator too
much, per perf.

On Sat, Nov 9, 2024, 8:48 AM Brice Dobry ***@***.***> wrote:

> ***@***.**** commented on this pull request.
> ------------------------------
>
> In stacks-common/src/types/chainstate.rs
> <https://github.com/stacks-network/stacks-core/pull/4997#discussion_r1835377699>
> :
>
> > +        let s = format!("{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}",
> +                          self.0[0],     self.0[1],       self.0[2],       self.0[3],
> +                          self.0[4],     self.0[5],       self.0[6],       self.0[7],
> +                          self.0[8],     self.0[9],       self.0[10],      self.0[11],
> +                          self.0[12],    self.0[13],      self.0[14],      self.0[15],
> +                          self.0[16],    self.0[17],      self.0[18],      self.0[19],
> +                          self.0[20],    self.0[21],      self.0[22],      self.0[23],
> +                          self.0[24],    self.0[25],      self.0[26],      self.0[27],
> +                          self.0[28],    self.0[29],      self.0[30],      self.0[31]);
> +        s
>
> pub fn to_string(&self) -> String {
>     self.0.iter().map(|byte| format!("{:02x}", byte)).collect()}
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/stacks-network/stacks-core/pull/4997#pullrequestreview-2425348048>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AADQJK4FVL5QOLAJUWV5NRDZ7YHEJAVCNFSM6AAAAABLKR7LICVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIMRVGM2DQMBUHA>
> .
> You are receiving this because your review was requested.Message ID:
> ***@***.***>
>

> The reason we *didn't* do that is because that invokes the allocator too much, per perf.

Ah, okay. Thanks for explaining.
Thanks for the reviews @jcnelson 🙏 
I marked as resolved all of the addressed comment
Re-requested Brice review as well since it was dismissed, but he already approved earlier.

@jcnelson I'm actually seeing something weird with the last test I added.

If I add a request after this [call](https://github.com/stacks-network/stacks-core/blob/7f3191b6aa697881d9dc088832dea4a4d1497d14/stackslib/src/net/api/tests/getclaritymetadata.rs#L256-L264), or if I add a previous request hitting this [code path](https://github.com/stacks-network/stacks-core/blob/a94abfda195ca653b6344924eaa62fba7bf2e250/stackslib/src/net/api/getclaritymetadata.rs#L121-L141) (such as a request `vm-metadata::9::invalid-contract-metadata-key`, the following request don't work and [this assertion fails](https://github.com/stacks-network/stacks-core/blob/a94abfda195ca653b6344924eaa62fba7bf2e250/stackslib/src/net/api/tests/mod.rs#L1080).

Is it possible that this code path breaks the test runner?
It works just fine in practice when calling this endpoint
@hugocaillard Can you push a test case so I can take a look?
@jcnelson 
I added this test: https://github.com/stacks-network/stacks-core/blob/c9d5295f1e668405609eb1c77ab3c363867c0581/stackslib/src/net/api/tests/getclaritymetadata.rs#L267-L274
Which is a copy of the first request (L217). 

But any request after the last one fails


Fix patch:
diff --git a/stackslib/src/net/rpc.rs b/stackslib/src/net/rpc.rs
index 3c1fec15c8..3a44de7953 100644
--- a/stackslib/src/net/rpc.rs
+++ b/stackslib/src/net/rpc.rs
@@ -115,8 +115,6 @@ pub struct ConversationHttp {
     pending_request: Option<ReplyHandleHttp>,
     /// outstanding response
     pending_response: Option<StacksHttpResponse>,
-    /// whether or not there's an error response pending
-    pending_error_response: bool,
     /// how much data to buffer (i.e. the socket's send buffer size)
     socket_send_buffer_size: u32,
 }
@@ -166,7 +164,6 @@ impl ConversationHttp {
             canonical_stacks_tip_height: None,
             pending_request: None,
             pending_response: None,
-            pending_error_response: false,
             keep_alive: true,
             total_request_count: 0,
             total_reply_count: 0,
@@ -228,15 +225,6 @@ impl ConversationHttp {
             );
             return Err(net_error::InProgress);
         }
-        if self.pending_error_response {
-            test_debug!(
-                "{:?},id={}: Error response is inflight",
-                &self.peer_host,
-                self.conn_id
-            );
-            return Err(net_error::InProgress);
-        }
-
         let handle = self.start_request(req)?;
 
         self.pending_request = Some(handle);
@@ -255,12 +243,12 @@ impl ConversationHttp {
             );
             return Err(net_error::InProgress);
         }
-        if self.pending_error_response {
-            // error already in-flight
-            return Ok(());
-        }
+        let (mut preamble, body_contents) = res.try_into_contents()?;
+        preamble.content_length = body_contents.content_length();
+        preamble.keep_alive = false;
 
-        let (preamble, body_contents) = res.try_into_contents()?;
+        // account for the request
+        self.total_request_count += 1;
 
         // make the relay handle. There may not have been a valid request in the first place, so
         // we'll use a relay handle (not a reply handle) to push out the error.
@@ -269,7 +257,6 @@ impl ConversationHttp {
         // queue up the HTTP headers, and then stream back the body.
         preamble.consensus_serialize(&mut reply)?;
         self.reply_streams.push_back((reply, body_contents, false));
-        self.pending_error_response = true;
         Ok(())
     }
 
@@ -388,11 +375,12 @@ impl ConversationHttp {
         if broken || (drained_handle && drained_stream) {
             // done with this stream
             test_debug!(
-                "{:?}: done with stream (broken={}, drained_handle={}, drained_stream={})",
+                "{:?}: done with stream (broken={}, drained_handle={}, drained_stream={}, do_keep_alive={})",
                 &self,
                 broken,
                 drained_handle,
-                drained_stream
+                drained_stream,
+                do_keep_alive,
             );
             self.total_reply_count += 1;
             self.reply_streams.pop_front();
@@ -482,6 +470,14 @@ impl ConversationHttp {
 
     /// Is the connection idle?
     pub fn is_idle(&self) -> bool {
+        test_debug!(
+            "{:?} is_idle? {},{},{},{}",
+            self,
+            self.pending_response.is_none(),
+            self.connection.inbox_len(),
+            self.connection.outbox_len(),
+            self.reply_streams.len()
+        );
         self.pending_response.is_none()
             && self.connection.inbox_len() == 0
             && self.connection.outbox_len() == 0
@@ -491,9 +487,13 @@ impl ConversationHttp {
     /// Is the conversation out of pending data?
     /// Don't consider it drained if we haven't received anything yet
     pub fn is_drained(&self) -> bool {
-        ((self.total_request_count > 0 && self.total_reply_count > 0)
-            || self.pending_error_response)
-            && self.is_idle()
+        test_debug!(
+            "{:?} is_drained? {},{}",
+            self,
+            self.total_request_count,
+            self.total_reply_count
+        );
+        self.total_request_count > 0 && self.total_reply_count > 0 && self.is_idle()
     }
 
     /// Should the connection be kept alive even if drained?
@@ -523,11 +523,6 @@ impl ConversationHttp {
         &mut self,
         node: &mut StacksNodeState,
     ) -> Result<Vec<StacksMessageType>, net_error> {
-        // if we have an in-flight error, then don't take any more requests.
-        if self.pending_error_response {
-            return Ok(vec![]);
-        }
-
         // handle in-bound HTTP request(s)
         let num_inbound = self.connection.inbox_len();
         let mut ret = vec![];
@@ -568,7 +563,6 @@ impl ConversationHttp {
                 }
                 StacksHttpMessage::Error(path, resp) => {
                     // new request, but resulted in an error when parsing it
-                    self.total_request_count += 1;
                     self.last_request_timestamp = get_epoch_time_secs();
                     let start_time = Instant::now();
                     self.reply_error(resp)?;
diff --git a/stackslib/src/net/server.rs b/stackslib/src/net/server.rs
index 3849b9b058..fdad3b85df 100644
--- a/stackslib/src/net/server.rs
+++ b/stackslib/src/net/server.rs
@@ -288,6 +288,7 @@ impl HttpPeer {
     /// Deregister a socket/event pair
     #[cfg_attr(test, mutants::skip)]
     pub fn deregister_http(&mut self, network_state: &mut NetworkState, event_id: usize) -> () {
+        test_debug!("Remove HTTP event {}", event_id);
         self.peers.remove(&event_id);
 
         match self.sockets.remove(&event_id) {
@@ -456,7 +457,7 @@ impl HttpPeer {
                                         "Failed to flush HTTP 400 to socket {:?}: {:?}",
                                         &client_sock, &e
                                     );
-                                    convo_dead = true;
+                                    // convo_dead = true;
                                 }
                             }
                             Err(e) => {
@@ -559,19 +560,11 @@ impl HttpPeer {
         let mut to_remove = vec![];
         let mut msgs = vec![];
         for event_id in &poll_state.ready {
-            if !self.sockets.contains_key(&event_id) {
+            let Some(client_sock) = self.sockets.get_mut(&event_id) else {
                 debug!("Rogue socket event {}", event_id);
                 to_remove.push(*event_id);
                 continue;
-            }
-
-            let client_sock_opt = self.sockets.get_mut(&event_id);
-            if client_sock_opt.is_none() {
-                debug!("No such socket event {}", event_id);
-                to_remove.push(*event_id);
-                continue;
-            }
-            let client_sock = client_sock_opt.unwrap();
+            };
 
             match self.peers.get_mut(event_id) {
                 Some(ref mut convo) => {


Test patch:
diff --git a/.github/workflows/p2p-tests.yml b/.github/workflows/p2p-tests.yml
index 2629efbbeb..1c33eca0fb 100644
--- a/.github/workflows/p2p-tests.yml
+++ b/.github/workflows/p2p-tests.yml
@@ -43,6 +43,10 @@ jobs:
           - net::tests::convergence::test_walk_star_15_org_biased
           - net::tests::convergence::test_walk_inbound_line_15
           - net::api::tests::postblock_proposal::test_try_make_response
+          - net::server::tests::test_http_10_threads_getinfo
+          - net::server::tests::test_http_10_threads_getblock
+          - net::server::tests::test_http_too_many_clients
+          - net::server::tests::test_http_slow_client
     steps:
       ## Setup test environment
       - name: Setup Test Environment