Skip to content

v2.3: Remove conditional feature as it is not supported (backport of #6577) #6607

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mergify wants to merge 2 commits into from
Closed

v2.3: Remove conditional feature as it is not supported (backport of #6577) #6607

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
de64390
Remove conditional feature as it is not supported (#6577)
mircea-c Jun 16, 2025
f1bd7bc
resolve merge conflicts
mircea-c Jun 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions svm/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,15 +53,12 @@ solana-svm-transaction = { workspace = true }
solana-system-interface = { workspace = true }
solana-sysvar-id = { workspace = true }
solana-timings = { workspace = true }
solana-transaction-context = { workspace = true }
solana-transaction-context = { workspace = true, features = ["debug-signature"] }
Copy link
Copy Markdown

@t-nelson t-nelson Jun 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the dependency should be redeclared with the feature flag in [dev-dependencies], not added to prod [dependencies]

Copy link
Copy Markdown

@mircea-c mircea-c Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Crate publishing fails if the feature is not added to [dependencies]

Copy link
Copy Markdown

@t-nelson t-nelson Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something else is wrong then. this should never be used in prod

Copy link
Copy Markdown

@t-nelson t-nelson Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p sure this should be gated by the cargo feature, not debug_assertions.

agave/svm/src/transaction_processor.rs

Lines 863 to 864 in 3281f61

#[cfg(debug_assertions)]
transaction_context.set_signature(tx.signature());

don't see any other usage of the tx-ctx symbols behind that feature

Copy link
Copy Markdown

@t-nelson t-nelson Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

honestly deleting the Signature bs might be the way to go? don't see it touched anywhere

Copy link
Copy Markdown

@t-nelson t-nelson Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Lichtso @LucasSte is this debug signature stuff dead code? can't find it used otherwise in monorepo or sdk

Copy link
Copy Markdown

@Lichtso Lichtso Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/anza-xyz/agave/blob/master/transaction-context/src/lib.rs#L180C5-L186C26

But yes, can be deleted if it causes too much trouble with dependencies.

Copy link
Copy Markdown

@t-nelson t-nelson Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/anza-xyz/agave/blob/master/transaction-context/src/lib.rs#L180C5-L186C26

sure. saw that. was more concerned that it's not actually read anywhere in the codebase other than implicitly through PartialEq, but don't actually see an instance of (a sanely named instantiation of) TransactionContext equality comparison in a debug_assert

But yes, can be deleted if it causes too much trouble with dependencies.

solana-transaction-error = { workspace = true }
solana-type-overrides = { workspace = true }
spl-generic-token = { workspace = true }
thiserror = { workspace = true }

[target.'cfg(debug_assertions)'.dependencies]
solana-transaction-context = { workspace = true, features = ["debug-signature"] }

[lib]
crate-type = ["lib"]
name = "solana_svm"
Expand Down
Loading