Fix/observer filters #5570

rdeioris · 2024-12-13T15:33:07Z

Description

This patch fixes the observer filtering system and adds tests for it.

Note that the patch includes a little change for fixing a race condition with 3 of the tests using the TEST_EVENT_OBSERVER_SKIP_RETRY variable (that was static and now is a thread local one)

Applicable issues

fixes Event dispatcher does not filter attachments when event_keys is configured #5558

Additional info (benefits, drawbacks, caveats)

Checklist

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)
New clarity functions have corresponding PR in clarity-benchmarking repo
New integration test(s) added to bitcoin-tests.yml

…cher unit tests

jbencin · 2024-12-16T19:09:50Z

testnet/stacks-node/src/event_dispatcher.rs

@@ -320,7 +322,7 @@ impl RewardSetEventPayload {
 }

 #[cfg(test)]
-static TEST_EVENT_OBSERVER_SKIP_RETRY: std::sync::Mutex<Option<bool>> = std::sync::Mutex::new(None);
+thread_local! { static TEST_EVENT_OBSERVER_SKIP_RETRY: RefCell<bool> = RefCell::new(false); }


Is it possible to use Cell here instead of RefCell?

Also can you add doc comment to this variable? Skip retry of what? I'm not familiar with this code so I don't know

Also, why does it have to be thread-local? If it doesn't have to be thread-local, can we just use the TestFlag struct?

@hstove tests are executed in parallel so there is a race condition between the set and clear of TEST_EVENT_OBSERVER_SKIP_RETRY (this caused the 3 related tests to often fail when executed together)

Ah that makes sense, thanks!

jcnelson · 2024-12-16T19:56:16Z

testnet/stacks-node/src/event_dispatcher.rs

-                .unwrap()
-                .unwrap_or(false)
-            {
+            if TEST_EVENT_OBSERVER_SKIP_RETRY.with(|v| *v.borrow()) {


Pervasively, instead of testing a global variable container for a global that should only be compiled for test code, can you create a helper function with an informative name, and a test and non-test body? Like this:

use std::sync::LazyLock; use crate::run_loop::neon::TestFlag; const TEST_EVENT_OBSERVER_SKIP_RETRY : LazyLock<TestFlag> = LazyLock::new(Testflag::default); /// Add a helpful docstring here #[cfg(any(test, feature = "testing"))] fn fault_injection_skip_observer_entry() -> bool { TEST_EVENT_OBSERVER_SKIP_RETRY.get() } #[cfg(not(any(test, feature = "testing")))] fn fault_injection_skip_observer_entry() -> bool { false }

obycode · 2024-12-16T19:50:41Z

testnet/stacks-node/src/event_dispatcher.rs

@@ -1272,6 +1266,12 @@ impl EventDispatcher {
        block_timestamp: Option<u64>,
        coinbase_height: u64,
    ) {
+        let interested_observers =


Should this get passed into create_dispatch_matrix_and_event_vector? and used there? Otherwise, I think this is still sending to observers that should have been filtered out.

Actually, I think this new code can just be removed.

obycode · 2024-12-16T19:56:05Z

testnet/stacks-node/src/event_dispatcher.rs

@@ -1592,7 +1592,8 @@ impl EventDispatcher {
    }

    pub fn process_new_attachments(&self, attachments: &[(AttachmentInstance, Attachment)]) {
-        let interested_observers: Vec<_> = self.registered_observers.iter().enumerate().collect();
+        let interested_observers = self.filter_observers(&self.mempool_observers_lookup, true);


I don't think this is the correct hash set to use for attachment events. None of the others look like a better option either. Maybe we should use any_event_observers_lookup, or maybe we should add a new one. I'm not entirely sure.

The specific issue (from the sbtc team that is actively using the event system) is about this /attachments/new event. Can we have a new event id dedicated to it?

testnet/stacks-node/src/event_dispatcher.rs

hstove · 2024-12-16T19:51:21Z

testnet/stacks-node/src/event_dispatcher.rs

+        let interested_observers =
+            self.filter_observers(&self.block_proposal_observers_lookup, true);
+        if interested_observers.is_empty() {
+            return;
+        }


Wrong filter here - you're filtering for only the block_proposal listeners, but this is for /new_block. Also, there isn't a filter for /new_block - it's sent by default (and needs to stay that way to be backwards compatible).

hstove · 2024-12-16T19:56:18Z

testnet/stacks-node/src/event_dispatcher.rs

-        let interested_observers: Vec<_> = self.registered_observers.iter().enumerate().collect();
+        let interested_observers = self.filter_observers(&self.mempool_observers_lookup, true);


This seems to be another copy/paste bug. There doesn't actually appear to be an attachments event key. I think it's best to probably filter for only the * event key here (self.any_event_observers_lookup), but I'm actually not sure.

hstove · 2024-12-16T19:58:45Z

testnet/stacks-node/src/event_dispatcher.rs

+    }
+
+    #[test]
+    fn test_event_dispatcher_tx_filtering_wrong() {


Can you add short comments above these tests? It takes me a bit to register exactly what's being tested in each of these. The code is good, it's just helpful to have a short explainer.

testnet/stacks-node/src/event_dispatcher.rs

hstove · 2024-12-16T20:01:22Z

testnet/stacks-node/src/event_dispatcher.rs

+        // Create a mock server
+        let mut server = mockito::Server::new();
+        let _m = server
+            .mock("POST", "/new_mempool_tx")


As I mentioned above - I think we need to always send /new_block proposals, so this test might be moot. Regardless, the path here should be /new_block.

Assuming we do make it so that /new_block is always sent, it would be good to modify this test such that we ensure /new_block is called event with an empty event_keys.

hstove · 2024-12-16T20:07:29Z

This brings up an interesting discussion about undocumented behavior here, and whether it's ok to break it.

Regarding /new_block, the undocumented behavior is that this is always sent. Signers rely on this, so removing this behavior would actually break signers and Stacks Blockchain APIs (unless they updated to use some not-currently-existing event key).

Regarding /attachments/new - this is pretty rarely used, especially now that BNS v2 is live. @zone117x , do all API event observers use the * event key? If so, I'm thinking that if we only sent attachments if you have *, this should be fine.

rdeioris · 2024-12-17T08:53:10Z

@hstove IMHO it seems a bit weird to expose a filtering system that cannot filter a bunch of events. What about some form of workaround that explicitely disables the new_block event (something like "!new_block")?

zone117x · 2024-12-17T15:44:47Z

This brings up an interesting discussion about undocumented behavior here, and whether it's ok to break it.

Regarding /new_block, the undocumented behavior is that this is always sent. Signers rely on this, so removing this behavior would actually break signers and Stacks Blockchain APIs (unless they updated to use some not-currently-existing event key).

Regarding /attachments/new - this is pretty rarely used, especially now that BNS v2 is live. @zone117x , do all API event observers use the * event key? If so, I'm thinking that if we only sent attachments if you have *, this should be fine.

Thanks for flagging @hstove. Yes it's common for API observers to use the * key. It's also common to use the STACKS_EVENT_OBSERVER env var which causes the node to also register with the * AFAIK.

Please do not break backward compatibility with these!

jcnelson · 2024-12-17T15:59:39Z

From the sprint call:

There's a case to be made for dropping support (deprecating) the current filter system and add a new opt-in filter system (this PR) where every event must be opted into. We get rid of the Any key in the new filter, and new events are only supported in the new opt-in filter. Nothing is on by default in the new filter system.

If both the old and new filters are configured, then the node errors out (the config file is invalid).

When this PR merges, we treat the old filter system as depreciated (add this to the changelog please).

One extensible option is to add the actual URL path instead of event keys.

rdeioris added 4 commits December 13, 2024 15:05

fixed observers filtering for new blocks and race condition in dispat…

2841e47

…cher unit tests

reverted mockito new_with_opts usage

4cef44e

added more tests for event dispatcher filtering

62812ce

fixed formatting

b559822

rdeioris marked this pull request as ready for review December 13, 2024 16:02

rdeioris requested a review from a team as a code owner December 13, 2024 16:02

aldur added this to the 3.1.0.0.3 milestone Dec 16, 2024

aldur requested review from jcnelson, obycode, jbencin and hstove December 16, 2024 16:20

jbencin reviewed Dec 16, 2024

View reviewed changes

jcnelson reviewed Dec 16, 2024

View reviewed changes

obycode reviewed Dec 16, 2024

View reviewed changes

hstove previously approved these changes Dec 16, 2024

View reviewed changes

fixed test url for test_event_dispatcher_nakamoto_block_filtering

9b6f72b

rdeioris dismissed hstove’s stale review via 9b6f72b December 17, 2024 08:56

fixed module using for tests

0eece4f

aldur assigned rdeioris Dec 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/observer filters #5570

Fix/observer filters #5570

rdeioris commented Dec 13, 2024 •

edited

Loading

jbencin Dec 16, 2024

jbencin Dec 16, 2024

hstove Dec 16, 2024

rdeioris Dec 17, 2024 •

edited

Loading

hstove Dec 17, 2024

jcnelson Dec 16, 2024

obycode Dec 16, 2024

obycode Dec 16, 2024

obycode Dec 16, 2024

rdeioris Dec 17, 2024

hstove Dec 16, 2024

hstove Dec 16, 2024

obycode Dec 16, 2024

hstove Dec 16, 2024

hstove Dec 16, 2024

hstove commented Dec 16, 2024

rdeioris commented Dec 17, 2024 •

edited

Loading

zone117x commented Dec 17, 2024

jcnelson commented Dec 17, 2024

		let interested_observers: Vec<_> = self.registered_observers.iter().enumerate().collect();
		let interested_observers = self.filter_observers(&self.mempool_observers_lookup, true);

Fix/observer filters #5570

Are you sure you want to change the base?

Fix/observer filters #5570

Conversation

rdeioris commented Dec 13, 2024 • edited Loading

Description

Applicable issues

Additional info (benefits, drawbacks, caveats)

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdeioris Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hstove commented Dec 16, 2024

rdeioris commented Dec 17, 2024 • edited Loading

zone117x commented Dec 17, 2024

jcnelson commented Dec 17, 2024

rdeioris commented Dec 13, 2024 •

edited

Loading

rdeioris Dec 17, 2024 •

edited

Loading

rdeioris commented Dec 17, 2024 •

edited

Loading