add shadow-compatible quinn-udp fallback#109
Conversation
| codegen-units = 1 | ||
|
|
||
| [patch.crates-io] | ||
| quinn-udp = { path = "rust/patch/quinn-udp" } |
There was a problem hiding this comment.
I'm completely lost - this is for testing, right? Why is it patched for everything - including prod builds?
There was a problem hiding this comment.
This pr should be in it own branch with a separate image tag its just for shadow related work not to be merged in main or devnet 5 branch
There was a problem hiding this comment.
I dropped a dm on Tg on this
There was a problem hiding this comment.
ah right, now I understand. However, I think maintaining two different versions can be too much burden. I think it should be possible to configure this via separate profile (https://doc.rust-lang.org/cargo/reference/profiles.html). Then, we can merge this, and build two images for all devnet-* branches
There was a problem hiding this comment.
Profiles can't toggle [patch.crates-io] they only control codegen flags, but we can drop the patch block from Cargo.toml and have CI apply it only for the shadow image via cargo build --release --config 'patch.crates-io.quinn-udp.path="rust/patch/quinn-udp"' so we have one branch, two images, no source divergence if this will be better?
There was a problem hiding this comment.
ye it is better, but still not good enough. First of all, better to keep this stuff in Makefile, so it can be executed locally. Second, you probably can do some trick like:
# Root Cargo.toml
[profile.shadow]
rustflags = ["--cfg", "shadow_mode"]
# P2P crate's Cargo.toml
[target.'cfg(shadow_mode)'.dependencies]
quinn-udp = { path = '..some path..' }
[target.'cfg(not(shadow_mode))'.dependencies]
quinn-udp = '..some version..'This will probably be more maintainable
There was a problem hiding this comment.
Thanks for the pointer, but that cfg-target swap only works when a crate directly depends on quinn-udp here we pull it in transitively through libp2p-quic, so there's nothing in our Cargo.toml files to swap, to redirect a transitive dependency, Cargo will only gives us [patch.crates-io], so I will keep the patch block out of source and apply it via --config from a make shadow-* target this way the same command works locally and in CI, while production stays untouched.
| zeroize = { workspace = true, features = ["derive"] } | ||
|
|
||
| [features] | ||
| shadow-integration = [] |
There was a problem hiding this comment.
this shouldn't be a feature, better to do it as a cfg, and enable in shadow profile automatically.
| } | ||
|
|
||
| #[cfg(feature = "shadow-integration")] | ||
| if crate::shadow_cost::fake_xmss() { |
There was a problem hiding this comment.
there is no point in this if check - you already have cfg option, that enables/disables this code. This just adds more complexity, without particular reason
There was a problem hiding this comment.
Keeping the runtime fake_xmss() check will help us preserves the option to run the shadow binary with real XMSS + sim-cost sleeps which is useful for baselining real prover behaviour under Shadow's virtual clock, which hardwiring fake XMSS to always be enabled would foreclose.
There was a problem hiding this comment.
oh i see, yeah, then makes sense to keep it
|
|
||
| pub const DEFAULT_FAKE_PROOF_SIZE: usize = 32 * 1024; | ||
|
|
||
| static FAKE_ENABLED: AtomicBool = AtomicBool::new(false); |
There was a problem hiding this comment.
this probably should always be true, as long as this file is enabled.
| } | ||
|
|
||
| #[cfg(feature = "shadow-integration")] | ||
| if crate::shadow_cost::fake_xmss() { |
There was a problem hiding this comment.
oh i see, yeah, then makes sense to keep it
2 participants
Shadow