Reloadable deny lists #1872
Reloadable deny lists #1872
Conversation
|
|
|
|
daniellehrner
requested a deployment
to
docker-build-and-e2e
GitHub Actions
Waiting
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| } catch (Exception e) { | ||
| log.error("Failed to reload shared deny lists", e); | ||
| return CompletableFuture.failedFuture(e); | ||
| } |
There was a problem hiding this comment.
Bug: Missing null check in reloadSharedDenyLists may cause NPE
The reloadSharedDenyLists() method accesses the static fields sharedDeniedAddresses, sharedBundleDeniedAddresses, sharedDeniedEvents, and sharedDeniedBundleEvents without checking if they are null. These fields are only initialized in initializeSharedDenyLists() which runs during start(). If reloadConfiguration() is called before start() completes, or after stop() but before restart, it will throw a NullPointerException. The previous implementation in LineaTransactionPoolValidatorPlugin had explicit initialization checks that have been removed.
There was a problem hiding this comment.
it should not possible to call the reload before start and after stop (otherwise it is an issue in Besu itself) so the null check is not required
| } catch (Exception e) { | ||
| log.error("Failed to reload shared deny lists", e); | ||
| return CompletableFuture.failedFuture(e); | ||
| } |
There was a problem hiding this comment.
Bug: Static deny list fields not reset in stop method
The stop() method resets sharedRegisterTasksDone and sharedStartTasksDone flags to false, but does not reset the static fields sharedDeniedAddresses, sharedBundleDeniedAddresses, sharedDeniedEvents, and sharedDeniedBundleEvents to null. This is inconsistent with other static fields like blockchainService and metricsSystem which are set to null. On restart, initializeSharedDenyLists() will create new ReloadableSet/ReloadableMap instances, potentially causing inconsistent state if any components still hold references to the old instances.
There was a problem hiding this comment.
I think it makes sense to reset the new fields too
There was a problem hiding this comment.
Pull request overview
This PR centralizes deny list management by introducing reloadable wrappers (ReloadableSet and ReloadableMap) and moving deny list state from individual plugins to a shared parent class (AbstractLineaSharedPrivateOptionsPlugin). This enables atomic, thread-safe configuration reloads across all plugins simultaneously.
- Introduces
ReloadableSet<T>andReloadableMap<K,V>for thread-safe, atomic swapping of deny list contents - Centralizes four shared deny lists in
AbstractLineaSharedPrivateOptionsPlugin: addresses, bundle addresses, events, and bundle events - Updates three plugins (
LineaTransactionSelectorPlugin,LineaTransactionPoolValidatorPlugin,LineaBundleEndpointsPlugin) to use shared deny lists instead of local state
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
ReloadableSet.java |
New wrapper class providing thread-safe atomic reloading of Set-based deny lists |
ReloadableMap.java |
New wrapper class providing thread-safe atomic reloading of Map-based deny lists |
AbstractLineaSharedPrivateOptionsPlugin.java |
Adds shared deny list initialization and centralized reloadSharedDenyLists() method |
LineaTransactionSelectorPlugin.java |
Removes local event deny list state, delegates to shared deny lists |
LineaTransactionPoolValidatorPlugin.java |
Removes local reload logic, delegates to shared reloadSharedDenyLists() |
LineaTransactionPoolValidatorFactory.java |
Replaces local AtomicReference with shared ReloadableSet, removes setter method |
LineaBundleEndpointsPlugin.java |
Removes local bundle address deny list state, uses shared deny lists |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public Set<K> keySet() { | ||
| return delegate.get().keySet(); | ||
| } | ||
|
|
||
| @NotNull | ||
| @Override | ||
| public Collection<V> values() { | ||
| return delegate.get().values(); | ||
| } | ||
|
|
||
| @NotNull | ||
| @Override | ||
| public Set<Entry<K, V>> entrySet() { | ||
| return delegate.get().entrySet(); | ||
| } |
There was a problem hiding this comment.
The keySet(), values(), and entrySet() methods return views over the underlying map obtained via delegate.get(). If reload() is called while code is iterating over these views, the views will continue to reference the old map (which is correct behavior for thread-safety). However, this behavior should be documented in the class-level Javadoc to clarify that these views are stable snapshots and won't see concurrent reloads.
| } | ||
|
|
||
| /** Reload the set from the configured source by invoking the reloader supplier. */ | ||
| public void reload() { |
There was a problem hiding this comment.
The reload() method is not thread-safe if called concurrently. If two threads call reload() simultaneously, both will invoke reloader.get() and call delegate.set(), potentially causing redundant expensive reload operations (e.g., file I/O). Consider either:
- Making
reload()synchronized to prevent concurrent execution - Using
compareAndSetin a loop to ensure only one thread performs the reload - Or documenting that callers are responsible for synchronizing reload calls
| public void reload() { | |
| public synchronized void reload() { |
| } | ||
|
|
||
| /** Reload the map from the configured source by invoking the reloader supplier. */ | ||
| public void reload() { |
There was a problem hiding this comment.
The reload() method is not thread-safe if called concurrently. If two threads call reload() simultaneously, both will invoke reloader.get() and call delegate.set(), potentially causing redundant expensive reload operations (e.g., file I/O). Consider either:
- Making
reload()synchronized to prevent concurrent execution - Using
compareAndSetin a loop to ensure only one thread performs the reload - Or documenting that callers are responsible for synchronizing reload calls
| public void reload() { | |
| public synchronized void reload() { |
There was a problem hiding this comment.
I think Cursor comment is a good one, in general it should not matter too much if the map is reloaded twice by concurrent threads, it is just a little waste, so probably it is sufficient to document that
| * | ||
| * <p>This class is immutable from the perspective of Set operations - all mutation methods throw | ||
| * {@link UnsupportedOperationException}. The underlying set can only be changed via {@link | ||
| * #reload(). |
There was a problem hiding this comment.
Missing closing brace } in Javadoc @link reference. The link should be {@link #reload()} instead of {@link #reload().
| * #reload(). | |
| * #reload()}. |
| public class ReloadableSet<T> implements Set<T> { | ||
| private final AtomicReference<Set<T>> delegate; | ||
| private final Supplier<Set<T>> reloader; | ||
|
|
||
| /** | ||
| * Creates a new ReloadableSet with an initial set and a reloader function. | ||
| * | ||
| * @param initial the initial set contents | ||
| * @param reloader a supplier that provides a new set when reload is called | ||
| */ | ||
| public ReloadableSet(final Set<T> initial, final Supplier<Set<T>> reloader) { | ||
| this.delegate = new AtomicReference<>(initial); | ||
| this.reloader = reloader; | ||
| } | ||
|
|
||
| /** Reload the set from the configured source by invoking the reloader supplier. */ | ||
| public void reload() { | ||
| delegate.set(reloader.get()); | ||
| } | ||
|
|
||
| /** | ||
| * Get the underlying AtomicReference. This is useful for validators that expect an | ||
| * AtomicReference<Set<T>>. | ||
| * | ||
| * @return the underlying AtomicReference | ||
| */ | ||
| public AtomicReference<Set<T>> getReference() { | ||
| return delegate; | ||
| } | ||
|
|
||
| // Delegate all Set read methods to the underlying set | ||
|
|
||
| @Override | ||
| public int size() { | ||
| return delegate.get().size(); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean isEmpty() { | ||
| return delegate.get().isEmpty(); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean contains(final Object o) { | ||
| return delegate.get().contains(o); | ||
| } | ||
|
|
||
| @NotNull | ||
| @Override | ||
| public Iterator<T> iterator() { | ||
| return delegate.get().iterator(); | ||
| } | ||
|
|
||
| @NotNull | ||
| @Override | ||
| public Object[] toArray() { | ||
| return delegate.get().toArray(); | ||
| } | ||
|
|
||
| @NotNull | ||
| @Override | ||
| public <T1> T1[] toArray(@NotNull final T1[] a) { | ||
| return delegate.get().toArray(a); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean containsAll(@NotNull final Collection<?> c) { | ||
| return delegate.get().containsAll(c); | ||
| } | ||
|
|
||
| // Mutation methods throw UnsupportedOperationException (immutable view) | ||
|
|
||
| @Override | ||
| public boolean add(final T t) { | ||
| throw new UnsupportedOperationException( | ||
| "ReloadableSet is immutable; use reload() to change contents"); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean remove(final Object o) { | ||
| throw new UnsupportedOperationException( | ||
| "ReloadableSet is immutable; use reload() to change contents"); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean addAll(@NotNull final Collection<? extends T> c) { | ||
| throw new UnsupportedOperationException( | ||
| "ReloadableSet is immutable; use reload() to change contents"); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean retainAll(@NotNull final Collection<?> c) { | ||
| throw new UnsupportedOperationException( | ||
| "ReloadableSet is immutable; use reload() to change contents"); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean removeAll(@NotNull final Collection<?> c) { | ||
| throw new UnsupportedOperationException( | ||
| "ReloadableSet is immutable; use reload() to change contents"); | ||
| } | ||
|
|
||
| @Override | ||
| public void clear() { | ||
| throw new UnsupportedOperationException( | ||
| "ReloadableSet is immutable; use reload() to change contents"); | ||
| } | ||
| } |
There was a problem hiding this comment.
The new ReloadableSet<T> class lacks test coverage. Given that this project has comprehensive test coverage for similar classes (e.g., validators in the txpoolvalidation package), tests should be added to verify:
- Thread-safe atomic swapping behavior during reload
- Proper delegation of read operations to the underlying set
- UnsupportedOperationException thrown for mutation operations
- Behavior when the reloader supplier throws exceptions
| public class ReloadableMap<K, V> implements Map<K, V> { | ||
| private final AtomicReference<Map<K, V>> delegate; | ||
| private final Supplier<Map<K, V>> reloader; | ||
|
|
||
| /** | ||
| * Creates a new ReloadableMap with an initial map and a reloader function. | ||
| * | ||
| * @param initial the initial map contents | ||
| * @param reloader a supplier that provides a new map when reload is called | ||
| */ | ||
| public ReloadableMap(final Map<K, V> initial, final Supplier<Map<K, V>> reloader) { | ||
| this.delegate = new AtomicReference<>(initial); | ||
| this.reloader = reloader; | ||
| } | ||
|
|
||
| /** Reload the map from the configured source by invoking the reloader supplier. */ | ||
| public void reload() { | ||
| delegate.set(reloader.get()); | ||
| } | ||
|
|
||
| /** | ||
| * Get the underlying AtomicReference. This is useful for components that expect an | ||
| * AtomicReference<Map<K,V>>. | ||
| * | ||
| * @return the underlying AtomicReference | ||
| */ | ||
| public AtomicReference<Map<K, V>> getReference() { | ||
| return delegate; | ||
| } | ||
|
|
||
| // Delegate all Map read methods to the underlying map | ||
|
|
||
| @Override | ||
| public int size() { | ||
| return delegate.get().size(); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean isEmpty() { | ||
| return delegate.get().isEmpty(); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean containsKey(final Object key) { | ||
| return delegate.get().containsKey(key); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean containsValue(final Object value) { | ||
| return delegate.get().containsValue(value); | ||
| } | ||
|
|
||
| @Override | ||
| public V get(final Object key) { | ||
| return delegate.get().get(key); | ||
| } | ||
|
|
||
| @NotNull | ||
| @Override | ||
| public Set<K> keySet() { | ||
| return delegate.get().keySet(); | ||
| } | ||
|
|
||
| @NotNull | ||
| @Override | ||
| public Collection<V> values() { | ||
| return delegate.get().values(); | ||
| } | ||
|
|
||
| @NotNull | ||
| @Override | ||
| public Set<Entry<K, V>> entrySet() { | ||
| return delegate.get().entrySet(); | ||
| } | ||
|
|
||
| // Mutation methods throw UnsupportedOperationException (immutable view) | ||
|
|
||
| @Override | ||
| public V put(final K key, final V value) { | ||
| throw new UnsupportedOperationException( | ||
| "ReloadableMap is immutable; use reload() to change contents"); | ||
| } | ||
|
|
||
| @Override | ||
| public V remove(final Object key) { | ||
| throw new UnsupportedOperationException( | ||
| "ReloadableMap is immutable; use reload() to change contents"); | ||
| } | ||
|
|
||
| @Override | ||
| public void putAll(@NotNull final Map<? extends K, ? extends V> m) { | ||
| throw new UnsupportedOperationException( | ||
| "ReloadableMap is immutable; use reload() to change contents"); | ||
| } | ||
|
|
||
| @Override | ||
| public void clear() { | ||
| throw new UnsupportedOperationException( | ||
| "ReloadableMap is immutable; use reload() to change contents"); | ||
| } | ||
| } |
There was a problem hiding this comment.
The new ReloadableMap<K,V> class lacks test coverage. Given that this project has comprehensive test coverage for similar classes, tests should be added to verify:
- Thread-safe atomic swapping behavior during reload
- Proper delegation of read operations to the underlying map
- UnsupportedOperationException thrown for mutation operations
- Behavior when the reloader supplier throws exceptions
| protected CompletableFuture<Void> reloadSharedDenyLists() { | ||
| try { | ||
| sharedDeniedAddresses.reload(); | ||
| sharedBundleDeniedAddresses.reload(); | ||
| sharedDeniedEvents.reload(); | ||
| sharedDeniedBundleEvents.reload(); | ||
| log.info("Successfully reloaded all shared deny lists"); | ||
| return CompletableFuture.completedFuture(null); | ||
| } catch (Exception e) { | ||
| log.error("Failed to reload shared deny lists", e); | ||
| return CompletableFuture.failedFuture(e); | ||
| } |
There was a problem hiding this comment.
The reloadSharedDenyLists() method reloads all four deny lists sequentially without atomicity. If one reload fails (e.g., sharedDeniedEvents.reload() throws an exception), the earlier reloads (e.g., sharedDeniedAddresses, sharedBundleDeniedAddresses) will have succeeded, leaving the system with partially updated deny lists. Consider:
- Wrapping all reload operations in a transaction-like pattern where you load all new values first, then atomically swap them all only if all succeed
- Or at minimum, document this behavior and its implications in the method's Javadoc
There was a problem hiding this comment.
Very good point, since deny lists are critical, we should avoid any possible inconsistent state
| public Iterator<T> iterator() { | ||
| return delegate.get().iterator(); | ||
| } |
There was a problem hiding this comment.
The iterator() method returns an iterator over the underlying set obtained via delegate.get(). If reload() is called while code is iterating over the set, the iterator will continue to reference the old set (which is correct behavior for thread-safety). However, this behavior should be documented in the class-level or method-level Javadoc to clarify that iterators are stable snapshots and won't see concurrent reloads.
| } | ||
|
|
||
| /** Reload the map from the configured source by invoking the reloader supplier. */ | ||
| public void reload() { |
There was a problem hiding this comment.
I think Cursor comment is a good one, in general it should not matter too much if the map is reloaded twice by concurrent threads, it is just a little waste, so probably it is sufficient to document that
| import java.util.Set; | ||
| import java.util.concurrent.atomic.AtomicReference; | ||
| import java.util.function.Supplier; | ||
| import org.jetbrains.annotations.NotNull; |
There was a problem hiding this comment.
as in Besu, could you use a more generic not null assertion?
| public ReloadableMap(final Map<K, V> initial, final Supplier<Map<K, V>> reloader) { | ||
| this.delegate = new AtomicReference<>(initial); | ||
| this.reloader = reloader; | ||
| } |
There was a problem hiding this comment.
Is there a reason to pass a separate initial value, instead of just using the supplier for it, like in the code below?
| public ReloadableMap(final Map<K, V> initial, final Supplier<Map<K, V>> reloader) { | |
| this.delegate = new AtomicReference<>(initial); | |
| this.reloader = reloader; | |
| } | |
| public ReloadableMap(final Supplier<Map<K, V>> reloader) { | |
| this.delegate = new AtomicReference<>(reloader.get()); | |
| this.reloader = reloader; | |
| } |
| public ReloadableSet(final Set<T> initial, final Supplier<Set<T>> reloader) { | ||
| this.delegate = new AtomicReference<>(initial); | ||
| this.reloader = reloader; | ||
| } |
| /** | ||
| * Get the underlying AtomicReference. This is useful for validators that expect an | ||
| * AtomicReference<Set<T>>. | ||
| * | ||
| * @return the underlying AtomicReference | ||
| */ | ||
| public AtomicReference<Set<T>> getReference() { | ||
| return delegate; | ||
| } |
There was a problem hiding this comment.
I suggest to remove this method, since it is now possible and cleaner to always pass the Map or the Set, as it was before introducing the AtomicReference
| final var validators = | ||
| new PluginTransactionPoolValidator[] { | ||
| new AllowedAddressValidator(bundleDeniedAddresses), | ||
| new AllowedAddressValidator(sharedBundleDeniedAddresses.getReference()), |
There was a problem hiding this comment.
with the nice re-loadable map, parameter passaga can be simplified again, for easier code, just passing the Map here
| new AllowedAddressValidator(sharedBundleDeniedAddresses.getReference()), | |
| new AllowedAddressValidator(sharedBundleDeniedAddresses), |
| } | ||
|
|
||
| @Override | ||
| public CompletableFuture<Void> reloadConfiguration() { |
There was a problem hiding this comment.
I would push the shared configuration reload mechanism even forward, to the point that individual plugins themselves are not responsible for the configuration lifecycle, since it is fully managed in the shared abstract class.
I know that you raised the point that in that way, we lose the granularity, and reloading a single plugin will trigger the reload of everything, but since Sequencer plugins are coupled together, currently it is more important to keep the configuration aligned, and avoiding bugs in a single plugin that could result in partial reloads, then we can well document that reload behavior, so if there are no stronger arguments against I prefer to push the reloadConfiguration in the abstract class and remove it in the plugin classes
| } catch (Exception e) { | ||
| log.error("Failed to reload shared deny lists", e); | ||
| return CompletableFuture.failedFuture(e); | ||
| } |
There was a problem hiding this comment.
it should not possible to call the reload before start and after stop (otherwise it is an issue in Besu itself) so the null check is not required
| } catch (Exception e) { | ||
| log.error("Failed to reload shared deny lists", e); | ||
| return CompletableFuture.failedFuture(e); | ||
| } |
There was a problem hiding this comment.
I think it makes sense to reset the new fields too
| protected CompletableFuture<Void> reloadSharedDenyLists() { | ||
| try { | ||
| sharedDeniedAddresses.reload(); | ||
| sharedBundleDeniedAddresses.reload(); | ||
| sharedDeniedEvents.reload(); | ||
| sharedDeniedBundleEvents.reload(); | ||
| log.info("Successfully reloaded all shared deny lists"); | ||
| return CompletableFuture.completedFuture(null); | ||
| } catch (Exception e) { | ||
| log.error("Failed to reload shared deny lists", e); | ||
| return CompletableFuture.failedFuture(e); | ||
| } |
There was a problem hiding this comment.
Very good point, since deny lists are critical, we should avoid any possible inconsistent state
Signed-off-by: F Bojarski <[email protected]>
Reload deny addresses and denied events globally in all plugins that use them at once
This PR implements issue(s) #
Checklist
Note
Centralizes address/event deny lists with reloadable shared structures and updates plugins to use them and reload in sync.
ReloadableSet<T>andReloadableMap<K,V>for atomic, thread-safe config reloading.AbstractLineaSharedPrivateOptionsPluginwith shared reloadable deny lists:sharedDeniedAddresses,sharedBundleDeniedAddresses.sharedDeniedEvents,sharedDeniedBundleEvents.reloadSharedDenyLists()to refresh all lists.LineaTransactionPoolValidatorFactory/Plugin: usesharedDeniedAddresses(viaReloadableSet.getReference()); remove local state/mutators; delegate reloads to shared method.LineaTransactionSelectorPlugin: usesharedDeniedEvents/sharedDeniedBundleEvents(viaReloadableMap.getReference()); delegate reloads.LineaBundleEndpointsPlugin: usesharedBundleDeniedAddressesand shared reload; remove local state.Written by Cursor Bugbot for commit 7e79af1. This will update automatically on new commits. Configure here.