Reject connections from outdated peers #2519
Conversation
There was a problem hiding this comment.
The core of this approach looks good. But I think we could tweak it to reduce its impact on other code.
Here are some suggestions:
Minimising interface changes:
- Define a minimal trait in
zebra-chain, and implement it on theBestTipHeightstruct inzebra-state- minimises changes in
zebra-chain - unfortunately the extra
tokiodependency causes CI issues, it's a bit of a long story
- minimises changes in
- The trait only needs a method to get the state's best tip height
- the caller doesn't care if the tip is finalized or not
- Create the
BestTipHeightwhen initializing the state- callers only have to change how they handle the return value
Minimising test changes:
- Move
create_state_servicetozebra_state::init_test- test modules that use the state can switch to
init_testinstead
- test modules that use the state can switch to
- Update the non-finalized watch channel at the end of
validate_and_commit- the
NonFinalizedStatedoesn't need to change at all
- the
- Make the watch channel optional in
FinalizedState- existing tests only need an extra
Noneargument
- existing tests only need an extra
- Make
with_best_tip_heightoptional inzebra_network- existing tests just work
Feel free to skip some of these suggestions, or make the changes a different way. I just want to reduce the overall size and risk of these changes.
|
a discussion (no related file):
I like this idea. I also wonder if it would make sense to have some sort of |
|
a discussion (no related file):
This turned out more complicated than I expected. The I added the feature, and that resulted in compilation issues because parts of the code are To try to make it compile, I had to reorganize part of the test code, and still add some Maybe this is something I should in a separate PR that should be merged before this one? |
|
a discussion (no related file):
You mean Or should I always do |
|
a discussion (no related file): There would only be one What do you think of this approach? I'm also kind of tempted to some sort of |
There was a problem hiding this comment.
Reviewable status: 0 of 23 files reviewed, 4 unresolved discussions (waiting on @jvff and @teor2345)
a discussion (no related file):
After thinking about this some more, I was wondering if it would make sense to change the approach, and have a
BestTipHeighttype that's the sender endpoint, instead of the receiver endpoint as it is now. That means that it would be an internal type, and the receiver endpoint would just be awatch::Receiver<block::Height>. TheBestTipHeightwould have aset_finalized_heightandset_non_finalized_height, and just make sure to update thewatch::Sender<block::Height>when appropriate.
I like this approach.
On Friday, I couldn't work out how to fit it into the existing state data structures and methods. But I think I can see how it could work now.
There would only be one
BestTipHeightinstance, insideStateService. Theset_finalized_heightwould be called inside thequeue_and_commit_non_finalizedmethod
Do you mean FinalizedState::queue_and_commit_finalized?
We know the latest finalized tip height once this loop finishes:
zebra/zebra-state/src/service/finalized_state.rs
Lines 136 to 140 in 544be14
We'd need to pass a &mut BestTipHeight to FinalizedState::queue_and_commit_finalized, but that seems like it would work.
and the
set_non_finalized_heightwould be called in thevalidate_and_commitmethod (usingself.mem.best_tip().0).
That would work, but we might want to wait until all the ready queued blocks have been processed, for consistency and performance.
We know the latest non-finalized tip height after this line in StateService::queue_and_commit_non_finalized:
zebra/zebra-state/src/service.rs
Line 152 in bfc3e4a
What do you think of this approach?
I think it keeps most of the changes contained in the state. And it reduces the number of channels.
Let's try it and see how it goes?
I'm also kind of tempted to some sort of
zebra_network::HandshakeValidatortrait and azebra_network::MinimumVersionCheckertype that wraps thewatch::Sender<block::Height>and implements the trait. That might help with testing, because we could use a defaultAccepttype ofHandshakeValidator. But this might be too much for now?
I think this is a good idea for a future ticket.
For now, we could just use Option<watch::Sender<block::Height>>, and use the default version if it is None.
There was a problem hiding this comment.
Reviewable status: 0 of 23 files reviewed, 4 unresolved discussions (waiting on @jvff and @teor2345)
a discussion (no related file):
Previously, jvff (Janito Vaqueiro Ferreira Filho) wrote…
Update the non-finalized watch channel at the end of
validate_and_commitYou mean
StateService::validate_and_commit? Won't this lead to updating thewatchchannel with the not-necessarily best chain tip?Or should I always do
best_tip.send(self.mem.best_tip().0)?
Good catch on this one - yes, we need to use the best tip height, not the latest or highest height.
jvff
force-pushed
the
reject-connections-from-outdated-peers
branch
from
12f3b6f
to
ee4e2fc
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 25 files reviewed, 1 unresolved discussion (waiting on @jvff and @teor2345)
a discussion (no related file):
Previously, teor2345 (teor) wrote…
After thinking about this some more, I was wondering if it would make sense to change the approach, and have a
BestTipHeighttype that's the sender endpoint, instead of the receiver endpoint as it is now. That means that it would be an internal type, and the receiver endpoint would just be awatch::Receiver<block::Height>. TheBestTipHeightwould have aset_finalized_heightandset_non_finalized_height, and just make sure to update thewatch::Sender<block::Height>when appropriate.I like this approach.
On Friday, I couldn't work out how to fit it into the existing state data structures and methods. But I think I can see how it could work now.
There would only be one
BestTipHeightinstance, insideStateService. Theset_finalized_heightwould be called inside thequeue_and_commit_non_finalizedmethodDo you mean
FinalizedState::queue_and_commit_finalized?We know the latest finalized tip height once this loop finishes:
zebra/zebra-state/src/service/finalized_state.rs
Lines 136 to 140 in 544be14
We'd need to pass a
&mut BestTipHeighttoFinalizedState::queue_and_commit_finalized, but that seems like it would work.and the
set_non_finalized_heightwould be called in thevalidate_and_commitmethod (usingself.mem.best_tip().0).That would work, but we might want to wait until all the ready queued blocks have been processed, for consistency and performance.
We know the latest non-finalized tip height after this line in
StateService::queue_and_commit_non_finalized:
zebra/zebra-state/src/service.rs
Line 152 in bfc3e4a
What do you think of this approach?
I think it keeps most of the changes contained in the state. And it reduces the number of channels.
Let's try it and see how it goes?
I'm also kind of tempted to some sort of
zebra_network::HandshakeValidatortrait and azebra_network::MinimumVersionCheckertype that wraps thewatch::Sender<block::Height>and implements the trait. That might help with testing, because we could use a defaultAccepttype ofHandshakeValidator. But this might be too much for now?I think this is a good idea for a future ticket.
For now, we could just use
Option<watch::Sender<block::Height>>, and use the default version if it isNone.
I updated the PR with this approach. It's still missing tests, so I'll keep it as draft for now. Let me know what you think 👍
There was a problem hiding this comment.
Reviewed 2 of 23 files at r1, 18 of 23 files at r2.
Reviewable status: 20 of 25 files reviewed, 8 unresolved discussions (waiting on @jvff and @teor2345)
a discussion (no related file):
Previously, jvff (Janito Vaqueiro Ferreira Filho) wrote…
I updated the PR with this approach. It's still missing tests, so I'll keep it as draft for now. Let me know what you think 👍
I think we're missing a few places where the finalized tip needs updating.
I'd like to focus on those in the next revision, so we know if this design will work.
I also noted some places where we could make the design more accurate, or reduce the size of this PR.
But they are only worth doing if we can get the finalized tip height to work.
zebra-network/src/isolated.rs, line 61 at r2 (raw file):
})) .with_user_agent(user_agent) .with_best_tip_height(best_tip_height)
Can we make with_best_tip_height optional, so we don't have to change this module at all?
(This also makes the changes to the network tests smaller.)
zebra-state/src/service.rs, line 75 at r2 (raw file):
pub fn new(config: Config, network: Network) -> (Self, watch::Receiver<block::Height>) { let (best_tip_height, best_tip_height_receiver) = BestTipHeight::new(); let disk = FinalizedState::new(&config, network);
What happens if the finalized state already has blocks in it?
zebra-state/src/service.rs, line 176 at r2 (raw file):
self.queued_blocks.prune_by_height(finalized_tip_height); self.best_tip_height .set_finalized_height(finalized_tip_height);
What about checkpointed blocks?
They get committed in FinalizedState::queue_and_commit_finalized, rather than this method.
zebra-state/src/service.rs, line 195 at r2 (raw file):
self.best_tip_height .set_best_non_finalized_height(self.mem.best_tip().map(|(height, _hash)| height));
For consistency, can we set both heights in the same place in queue_and_commit_non_finalized?
If we set both heights in the same place, future changes should account for them both.
zebra-state/src/service/best_tip_height.rs, line 5 at r2 (raw file):
Documentation typo - could be confusing.
/// A helper type to determine the best chain tip block height.
zebra-state/src/service/best_tip_height.rs, line 14 at r2 (raw file):
non_finalized: Option<block::Height>, sender: watch::Sender<block::Height>, // TODO: Replace this with a `watch::Sender::borrow` call once Tokio is updated to 1.0.0
👍
zebra-state/src/service/best_tip_height.rs, line 30 at r2 (raw file):
non_finalized: None, sender, active_value: genesis_height,
When Zebra starts with an empty state, the finalized tip height is actually None, because we haven't downloaded the genesis block yet.
Can we use Option<block::Height> and initialize it to None?
(This comment applies to the channel and the BestTipHeight's fields.)
zebra-state/tests/basic.rs, line 84 at r2 (raw file):
Optional: Is this code is equivalent to zebra_state::init_test?
It uses a temporary directory which auto-deletes, the same as the ephemeral config.
Quoted 5 lines of code…
let storage_guard = TempDir::new("")?; let cache_dir = storage_guard.path().to_owned(); let (service, _) = zebra_state::init( Config { cache_dir,
jvff
force-pushed
the
reject-connections-from-outdated-peers
branch
from
ee4e2fc
to
799e4d4
Compare
There was a problem hiding this comment.
Reviewable status: 9 of 25 files reviewed, 7 unresolved discussions (waiting on @jvff and @teor2345)
zebra-state/src/service.rs, line 75 at r2 (raw file):
Previously, teor2345 (teor) wrote…
What happens if the finalized state already has blocks in it?
Done.
zebra-state/src/service.rs, line 176 at r2 (raw file):
Previously, teor2345 (teor) wrote…
What about checkpointed blocks?
They get committed in
FinalizedState::queue_and_commit_finalized, rather than this method.
Done.
zebra-state/src/service.rs, line 195 at r2 (raw file):
Previously, teor2345 (teor) wrote…
For consistency, can we set both heights in the same place in
queue_and_commit_non_finalized?If we set both heights in the same place, future changes should account for them both.
Done.
zebra-state/src/service/best_tip_height.rs, line 5 at r2 (raw file):
Previously, teor2345 (teor) wrote…
Documentation typo - could be confusing.
/// A helper type to determine the best chain tip block height.
Done.
zebra-state/src/service/best_tip_height.rs, line 30 at r2 (raw file):
Previously, teor2345 (teor) wrote…
When Zebra starts with an empty state, the finalized tip height is actually
None, because we haven't downloaded the genesis block yet.Can we use
Option<block::Height>and initialize it toNone?
(This comment applies to the channel and the BestTipHeight's fields.)
Done, but I kept the setter method to receive just a block::Height instead of Option<block::Height>. My reasoning is that the finalized height should only be None while the finalized state is empty, and that it should never be cleared (i.e., set to None). But I can change that if this is incorrect or unnecessarily cautious.
zebra-state/tests/basic.rs, line 84 at r2 (raw file):
Previously, teor2345 (teor) wrote…
Optional: Is this code is equivalent to
zebra_state::init_test?It uses a temporary directory which auto-deletes, the same as the ephemeral config.
let storage_guard = TempDir::new("")?; let cache_dir = storage_guard.path().to_owned(); let (service, _) = zebra_state::init( Config { cache_dir,
Done. Yep, it's exactly the same! However, I did notice that the TempDir guard in the ephemeral Config is dropped really soon. Doesn't that mean that the directory is deleted before it's used? 🤔
zebra-network/src/isolated.rs, line 61 at r2 (raw file):
Previously, teor2345 (teor) wrote…
Can we make
with_best_tip_heightoptional, so we don't have to change this module at all?(This also makes the changes to the network tests smaller.)
Done.
There was a problem hiding this comment.
Reviewable status: 9 of 25 files reviewed, 7 unresolved discussions (waiting on @jvff and @teor2345)
a discussion (no related file):
Previously, teor2345 (teor) wrote…
I think we're missing a few places where the finalized tip needs updating.
I'd like to focus on those in the next revision, so we know if this design will work.I also noted some places where we could make the design more accurate, or reduce the size of this PR.
But they are only worth doing if we can get the finalized tip height to work.
Updated 👍
There was a problem hiding this comment.
Reviewed 16 of 16 files at r3.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @jvff and @teor2345)
a discussion (no related file):
Previously, jvff (Janito Vaqueiro Ferreira Filho) wrote…
Updated 👍
This design and code looks good.
Required changes:
- we're using the heights of valid and invalid finalized blocks, but we only want valid block heights
- add one or two tests
I'd like to merge on my Monday, so you can move on to other work on your Monday.
We will add a lot more tests as part of the mempool work, so we just need basic coverage for now.
I also made a bunch of very minor API style suggestions. If you have time, feel free to fix them, otherwise feel free to ignore them, or open a follow-up ticket. Some of the changes might make testing easier.
zebra-network/src/peer/handshake.rs, line 428 at r3 (raw file):
Optional: Can we just pass self.best_tip_height to Handshake, instead of creating a fake channel?
If we update the channel type as part of the mempool work, we'll also have to make changes to the fake channel's type.
That's ok, and we could merge this code as-is.
But I'd like to avoid the extra work - unless Option<watch::Receiver<Option<Height>> is much harder to use.
self.best_tip_height,
zebra-network/src/peer/handshake.rs, line 585 at r3 (raw file):
// network upgrades and could lead to chain forks or slower block propagation. let height = best_tip_height.borrow().unwrap_or(block::Height(0)); let min_version = Version::min_remote_for_height(config.network, height);
Optional: Can we handle Option<Height> in min_remote_for_height instead?
We could merge this code as-is - it works fine, and returns the correct minimum version for None.
But we're also going to need to handle None heights when we disconnect from existing peers, so I'd prefer to do that all in the same place. And I'd like to avoid the habit of substituting Height(0) for None, it will get us into trouble eventually!
There's no need to push Option<Height> any further up the call stack, because the correct version for an empty state is just Zebra's minimum network protocol version: Version::initial_min_for_network.
let min_version = Version::min_remote_for_height(config.network, best_tip_height.borrow());
zebra-network/src/peer_set/initialize/tests/vectors.rs, line 115 at r3 (raw file):
Optional: Can we just skip creating the channel here? It's now optional.
I'd like to avoid creating an unused channel for every zebra-network test we write.
Again, this is just a cleanup, and we could merge the code if you want.
let (_peer_service, address_book) = init(config, inbound_service, None).await;
zebra-state/src/service.rs, line 679 at r3 (raw file):
self.pending_utxos.check_against(&finalized.new_outputs); self.disk.queue_and_commit_finalized((finalized, rsp_tx)); self.best_tip_height.set_finalized_height(finalized_height);
Blocker: This code updates the finalized tip height for valid and invalid blocks.
Before and after NU5, block commits can fail due to database errors.
After NU5, malicious peers can modify the authorizing data in v5 transactions without changing the transaction ID. So we need to check the block commitment in the finalized state, and return an error if the check fails. For more details, see the last paragraph of:
#2134 (comment)
Can we use the finalized state's tip height here, rather than the block height?
zebra-state/src/service/best_tip_height.rs, line 30 at r2 (raw file):
Previously, jvff (Janito Vaqueiro Ferreira Filho) wrote…
Done, but I kept the setter method to receive just a
block::Heightinstead ofOption<block::Height>. My reasoning is that the finalized height should only beNonewhile the finalized state is empty, and that it should never be cleared (i.e., set toNone). But I can change that if this is incorrect or unnecessarily cautious.
I think the required fix to the finalized tip update might impact the types here.
But you're correct, both the finalized and non-finalized state are None when empty. Then after they are populated, they are never reset to None.
So it would be ok to take a Height and ignore None in the caller, or take Option<Height> and ignore it in these methods.
I have a slight preference for consistent method arguments, and for ignoring None inside the methods. That way, the "ignore None" code is all in the same module.
But I really don't mind either way, as long as the code is correct.
zebra-state/src/service/best_tip_height.rs, line 60 at r3 (raw file):
/// height that was sent. fn update(&mut self) { let new_value = self.non_finalized.max(self.finalized);
Required Test: Option::max is tricky to get right, so let's make sure that we get Some if one of the heights is None.
I'm pretty sure that max(None, Some) is Some, but min(None, Some) is None, because Option is defined as { None, Some(T) }.
For details, see: https://doc.rust-lang.org/std/cmp/trait.Ord.html#derivable
zebra-state/tests/basic.rs, line 84 at r2 (raw file):
Previously, jvff (Janito Vaqueiro Ferreira Filho) wrote…
Done. Yep, it's exactly the same! However, I did notice that the
TempDirguard in the ephemeralConfigis dropped really soon. Doesn't that mean that the directory is deleted before it's used? 🤔
We just use TempDir to generate the path - the directory is deleted immediately. Then RocksDB creates the directory, and the finalized state manually deletes it on drop.
It's a bit messy, but we were trying to do low-risk changes at the time. Also I think I understand lifetimes much better now.
jvff
force-pushed
the
reject-connections-from-outdated-peers
branch
from
799e4d4
to
a9b1533
Compare
There was a problem hiding this comment.
Reviewable status: 17 of 29 files reviewed, 7 unresolved discussions (waiting on @jvff and @teor2345)
a discussion (no related file):
Previously, teor2345 (teor) wrote…
This design and code looks good.
Required changes:
- we're using the heights of valid and invalid finalized blocks, but we only want valid block heights
- add one or two tests
I'd like to merge on my Monday, so you can move on to other work on your Monday.
We will add a lot more tests as part of the mempool work, so we just need basic coverage for now.
I also made a bunch of very minor API style suggestions. If you have time, feel free to fix them, otherwise feel free to ignore them, or open a follow-up ticket. Some of the changes might make testing easier.
I updated the PR to apply the suggestions and to start writing the tests. Unfortunately, I haven't finished them yet :( But I pushed the current status and I added a comment on the test that's incomplete with what I'm trying to solve.
Let me know what you think of the tests. I thought that it might make sense to add a similar test that finalizes and commits blocks in an unordered way, but I'm not sure if that's too much and how much work it would take 🤔
zebra-network/src/peer/handshake.rs, line 428 at r3 (raw file):
Previously, teor2345 (teor) wrote…
Optional: Can we just pass
self.best_tip_heighttoHandshake, instead of creating a fake channel?If we update the channel type as part of the mempool work, we'll also have to make changes to the fake channel's type.
That's ok, and we could merge this code as-is.But I'd like to avoid the extra work - unless
Option<watch::Receiver<Option<Height>>is much harder to use.self.best_tip_height,
Done.
zebra-network/src/peer/handshake.rs, line 585 at r3 (raw file):
Previously, teor2345 (teor) wrote…
Optional: Can we handle
Option<Height>inmin_remote_for_heightinstead?We could merge this code as-is - it works fine, and returns the correct minimum version for
None.But we're also going to need to handle
Noneheights when we disconnect from existing peers, so I'd prefer to do that all in the same place. And I'd like to avoid the habit of substitutingHeight(0)forNone, it will get us into trouble eventually!There's no need to push
Option<Height>any further up the call stack, because the correct version for an empty state is just Zebra's minimum network protocol version:Version::initial_min_for_network.let min_version = Version::min_remote_for_height(config.network, best_tip_height.borrow());
Done.
zebra-network/src/peer_set/initialize/tests/vectors.rs, line 115 at r3 (raw file):
Previously, teor2345 (teor) wrote…
Optional: Can we just skip creating the channel here? It's now optional.
I'd like to avoid creating an unused channel for every zebra-network test we write.
Again, this is just a cleanup, and we could merge the code if you want.
let (_peer_service, address_book) = init(config, inbound_service, None).await;
Done.
zebra-state/src/service.rs, line 679 at r3 (raw file):
Previously, teor2345 (teor) wrote…
Blocker: This code updates the finalized tip height for valid and invalid blocks.
Before and after NU5, block commits can fail due to database errors.
After NU5, malicious peers can modify the authorizing data in v5 transactions without changing the transaction ID. So we need to check the block commitment in the finalized state, and return an error if the check fails. For more details, see the last paragraph of:
#2134 (comment)Can we use the finalized state's tip height here, rather than the block height?
Done.
zebra-state/src/service/best_tip_height.rs, line 60 at r3 (raw file):
Previously, teor2345 (teor) wrote…
Required Test:
Option::maxis tricky to get right, so let's make sure that we getSomeif one of the heights isNone.I'm pretty sure that
max(None, Some)isSome, butmin(None, Some)isNone, becauseOptionis defined as{ None, Some(T) }.For details, see: https://doc.rust-lang.org/std/cmp/trait.Ord.html#derivable
Done.
zebra-state/src/service/tests.rs, line 311 at r4 (raw file):
); todo!("Commit each finalized block");
FYI: Still haven't finished this test. It still needs to:
- Commit finalized blocks (I guess I just need to send a
Request::CommitFinalizedBlock?) - Commit non-finalized blocks (
Request::CommitBlock?) - Finalize blocks (is there a way to force this, or do I have to send the block again with
Request::CommitFinalizedBlock?) - Get it running in an
asyncenvironment. I tried to get the whole test working as anasync fnand#[tokio::test], but I couldn't get it to work. I'll have to investigate more later.
There was a problem hiding this comment.
Reviewed 12 of 12 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jvff and @teor2345)
a discussion (no related file):
Previously, jvff (Janito Vaqueiro Ferreira Filho) wrote…
I updated the PR to apply the suggestions and to start writing the tests. Unfortunately, I haven't finished them yet :( But I pushed the current status and I added a comment on the test that's incomplete with what I'm trying to solve.
Let me know what you think of the tests. I thought that it might make sense to add a similar test that finalizes and commits blocks in an unordered way, but I'm not sure if that's too much and how much work it would take 🤔
I think the goals of the state chain test are a bit too big for the time we have available.
Can you focus that test on the new best tip interface?
We already have tests for unordered commits, and for the best tip of multiple chains, so there's no need to re-test that code.
We've made the zebra-network changes really small, so I don't think we need to test them right now.
Can you open a ticket for a proptest for the minimum peer version at various heights?
zebra-state/src/service/tests.rs, line 311 at r4 (raw file):
I'd suggest using the StateService and its methods directly, like the existing tests.
Commit finalized blocks (I guess I just need to send a Request::CommitFinalizedBlock?)
Commit non-finalized blocks (Request::CommitBlock?)
zebra/zebra-state/src/service/non_finalized_state/tests/prop.rs
Lines 164 to 172 in f9c0620
Finalize blocks (is there a way to force this, or do I have to send the block again with Request::CommitFinalizedBlock?)
If you use PreparedChain, then every chain is longer than MIN_TRANSPARENT_COINBASE_MATURITY.
So some blocks will automatically be finalized when count is large enough.
Get it running in an async environment. I tried to get the whole test working as an async fn and #[tokio::test], but I couldn't get it to work. I'll have to investigate more later.
Can we avoid this complexity in this test, by using the StateService and its methods directly?
We already have tests for the state's async methods.
If your tests trigger the minimum non-finalized height assertion, feel free to move that assertion into the Request::CommitBlock handler. (Or some intermediate method.)
zebra-state/src/service/tests.rs, line 358 at r4 (raw file):
} fn ordered_blockchain_state_strategy() -> impl Strategy<
I'm not sure if the ordered_blockchain_state_strategy is going to pass state contextual validation. Even if it does right now, it probably won't continue to pass as we add new features.
Can you use a single PreparedChain instead, with no forks?
It's used by existing tests, so it will keep on working when we make changes.
You can use the random count argument to choose the number of blocks to use from the chain:
Generating a blockchain with arbitrary forks is a good idea, but it belongs in another ticket, when we have more time.
|
zebra-state/src/service/tests.rs, line 311 at r4 (raw file): Previously, teor2345 (teor) wrote…
I'm having some trouble with this test. I'm running into a failed assertion: |
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jvff and @teor2345)
zebra-state/src/service/tests.rs, line 311 at r4 (raw file):
Previously, jvff (Janito Vaqueiro Ferreira Filho) wrote…
I'm having some trouble with this test. I'm running into a failed assertion:
contextual validation requires at least 28 (POW_AVERAGING_WINDOW + POW_MEDIAN_BLOCK_SPAN) blocks:(
Can you move that assertion and the minimum non-finalized height assertion to the Request::CommitBlock handler?
zebra/zebra-state/src/service.rs
Line 638 in 4f96a4b
It's not a perfect solution. But it keeps checking those assertions for StateService as tower::Service callers, which is what we need in production.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r5.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @teor2345)
zebra-state/src/service/tests.rs, line 311 at r4 (raw file):
Previously, teor2345 (teor) wrote…
Can you move that assertion and the minimum non-finalized height assertion to the
Request::CommitBlockhandler?
zebra/zebra-state/src/service.rs
Line 638 in 4f96a4b
It's not a perfect solution. But it keeps checking those assertions for
StateService as tower::Servicecallers, which is what we need in production.
We're almost there, it looks like you missed moving the assertion that's actually failing:
zebra/zebra-state/src/service.rs
Lines 213 to 220 in 7218aec
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jvff and @teor2345)
zebra-state/src/service/tests.rs, line 282 at r5 (raw file):
Just noting that this comment will need updating after the test is finished.
/// Test that the best tip height is updated accordingly.
zebra-state/src/service/tests.rs, line 294 at r5 (raw file):
The genesis block needs to be committed to the finalized state, so you'll have to do something like this before splitting the chain:
let count = max(count, 1);
There was a problem hiding this comment.
Reviewed 1 of 3 files at r6.
Reviewable status: 29 of 31 files reviewed, 4 unresolved discussions (waiting on @jvff and @teor2345)
zebra-state/src/service/tests.rs, line 315 at r5 (raw file):
This line and the similar line below the non-finalized loop seem redundant, because the check happens at the end of each loop, after expected_height is modified:
prop_assert_eq!(*best_tip_height.borrow(), expected_height);
jvff
force-pushed
the
reject-connections-from-outdated-peers
branch
2 times, most recently
from
b6a2395
to
f5e7b24
Compare
There was a problem hiding this comment.
Reviewable status: 28 of 31 files reviewed, 5 unresolved discussions (waiting on @jvff and @teor2345)
zebra-state/src/service/tests.rs, line 311 at r4 (raw file):
Previously, teor2345 (teor) wrote…
We're almost there, it looks like you missed moving the assertion that's actually failing:
zebra/zebra-state/src/service.rs
Lines 213 to 220 in 7218aec
Done.
zebra-state/src/service/tests.rs, line 358 at r4 (raw file):
Previously, teor2345 (teor) wrote…
I'm not sure if the
ordered_blockchain_state_strategyis going to pass state contextual validation. Even if it does right now, it probably won't continue to pass as we add new features.Can you use a single
PreparedChaininstead, with no forks?
It's used by existing tests, so it will keep on working when we make changes.You can use the random
countargument to choose the number of blocks to use from the chain:
Generating a blockchain with arbitrary forks is a good idea, but it belongs in another ticket, when we have more time.
Done, updated to use the chain in the test vectors. I'll open a ticket for testing with arbitrary forks 👍
zebra-state/src/service/tests.rs, line 282 at r5 (raw file):
Previously, teor2345 (teor) wrote…
Just noting that this comment will need updating after the test is finished.
/// Test that the best tip height is updated accordingly.
Done.
zebra-state/src/service/tests.rs, line 294 at r5 (raw file):
Previously, teor2345 (teor) wrote…
The genesis block needs to be committed to the finalized state, so you'll have to do something like this before splitting the chain:
let count = max(count, 1);
Done, in the continuous_empty_blocks_from_test_vectors helper function.
zebra-state/src/service/tests.rs, line 315 at r5 (raw file):
Previously, teor2345 (teor) wrote…
This line and the similar line below the non-finalized loop seem redundant, because the check happens at the end of each loop, after
expected_heightis modified:prop_assert_eq!(*best_tip_height.borrow(), expected_height);
Done.
jvff
force-pushed
the
reject-connections-from-outdated-peers
branch
from
f5e7b24
to
d1ab45b
Compare
Make it available so that the best tip height can be watched.
After blocks from the queue are finalized and committed to disk, update the finalized block height.
Update the value of the best non-finalized chain tip block height after a new block is committed to the non-finalized state.
When `FinalizedState` is first created, it loads the state from persistent storage, and the finalized tip height is updated. Therefore, the `best_tip_height` must be notified of the initial value.
When a checkpointed block is commited, it bypasses the non-finalized state, so there's an extra place where the finalized height has to be updated.
It can be configured using the `Builder::with_best_tip_height`. It's currently not used, but it will be used to determine if a connection to a remote peer should be rejected or not based on that peer's protocol version.
Without it the handshake service can't properly enforce the minimum network protocol version from peers. Zebrad obtains the best tip height endpoint from `zebra_state`, and the test vectors simply use a dummy endpoint that's fixed at the genesis height.
The protocol version negotiation code will reject connections to peers if they are using an old protocol version. An old version is determined based on the current known best chain tip height.
Fallback to the genesis height in `None` is specified.
Avoid connecting to peers that are on protocol versions that don't recognize a network update.
Describe why it's a security issue above the check.
Check if initially there is no best tip height.
After applying a list of random updates where each one either sets the finalized height or the non-finalized height, check that the best tip height is the maximum of the most recently set finalized height and the most recently set non-finalized height.
A small refactor to make testing easier. The handling of requests for committing non-finalized and finalized blocks is now more consistent.
Refactor to move into a separate method some assertions that are done before a block is validated. This is to allow moving these assertions more easily to simplify testing.
It's also checked in `zebra_state::service::check::block_is_contextually_valid`, and it was getting in the way of tests that received a gossiped block before finalizing enough blocks.
Splits a chain loaded from the test vectors in two parts, containing the blocks to finalize and the blocks to keep in the non-finalized state.
Create a mock blockchain state, with a chain of finalized blocks and a chain of non-finalized blocks. Commit all the blocks appropriately, and verify that the best tip height is updated.
jvff
force-pushed
the
reject-connections-from-outdated-peers
branch
from
032611c
to
73a96e0
Compare
There was a problem hiding this comment.
Reviewable status: 29 of 31 files reviewed, 2 unresolved discussions (waiting on @teor2345)
zebra-state/src/service.rs, line 536 at r9 (raw file):
Previously, teor2345 (teor) wrote…
To be clear, we only want to delete the
POW_AVERAGING_WINDOW + POW_MEDIAN_BLOCK_SPANassertion.The
block.height > self.network.mandatory_checkpoint_height(),assertion is checked by the chain verifier, so it can stay.
Done.
zebra-state/src/service/tests.rs, line 311 at r4 (raw file):
Previously, teor2345 (teor) wrote…
This didn't quite work the way I expected. Now Zebra panics if a peer gossips a new block (via the inbound service) before it commits 28 blocks.
Let's delete this specific assertion - it's redundant anyway. I've left a comment there as well.
The application panicked (crashed). Message: contextual validation requires at least 28 (POW_AVERAGING_WINDOW + POW_MEDIAN_BLOCK_SPAN) blocks Location: zebra-state/src/service.rs:536 Metadata: Zcash network: Mainnet git commit: 5de4530 ... ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ SPANTRACE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 0: zebra_state::service::state at zebra-state/src/service.rs:672 1: zebrad::components::inbound::downloads::download_and_verify with hash=00000000013537fd78bb7ac86324cabf77d6c189cc867abec5d5d0e1a0193f67 at zebrad/src/components/inbound/downloads.rs:170 2: zebrad::components::inbound::inbound at zebrad/src/components/inbound.rs:226
Done.
zebra-state/src/service/tests.rs, line 322 at r9 (raw file):
Previously, teor2345 (teor) wrote…
Nit: typo
the genesis block
Done.
zebra-state/src/service/tests.rs, line 352 at r9 (raw file):
Previously, teor2345 (teor) wrote…
You might want
1..=blocks.len()here, so that we test with an entirely finalized chain:
https://doc.rust-lang.org/std/vec/struct.Vec.html#method.split_offThis isn't particularly important though.
let finalized_blocks_count = 1..blocks.len(); ... let non_finalized_blocks = blocks.split_off(finalized_blocks_count);
Done.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r10.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @jvff)
|
Should this one have closed #1334 or is there a follow up PR to be done? |
Motivation
When NU5 activates, there may still be peers on the network that are running outdated software. This means that they would be advertising themselves with support for an older network protocol version. In order to incentivize peers to upgrade their software and improve network health, it would be best for Zebra nodes to automatically only accept connections from peers that support NU5 after NU5 is activated.
Specifications
ZIP-201 details how a node rejects connections from peers that are on an older network protocol and therefore don't support a network upgrade when it's activated.
Designs
#1334 outlines an initial design.
Solution
The solution implemented here adds a
BestTipHeighthelper type. This type helps to determine the best tip height based on the finalized tip height and the tip height of the best non-finalized chain. It then sends the block height to awatchchannel. Thezebra-statenow provides the best tip heightwatch::Receiverendpoint when it is initialized.The
zebra-networkcrate can now receive the best tip heightwatch::Receiverendpoint and will determine the minimum supported network protocol version based on its value. It will automatically reject peers that have a network protocol version less than the calculated minimum network protocol version.The
zebradcrate was updated to forward thewatch::Receiverendpoint created byzebra-stateintozebra-network.The PR includes a few unit tests for the
BestTipHeighttype, as well as a slightly broader property test that checks if the best tip height is updated by the state service.Review
@teor2345 has been reviewing and helping with this PR.
Reviewer Checklist
Follow Up Work
active_valueonce Tokio is updated to version 1.0.0 (Remove redundant watch values once our Tokio dependency is updated #2573)This change is