Skip to content

Remove unused florestad and wire features #486

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

Jump to bottom
Merged
merged 1 commit into from
May 16, 2025
Merged

Remove unused florestad and wire features #486

merged 1 commit into from
May 16, 2025

Conversation

@JoseSK999
Copy link
Contributor

@JoseSK999 JoseSK999 commented May 16, 2025 •
edited
Loading

What is the purpose of this pull request?

  • Bug fix
  • Documentation update
  • New feature
  • Test
  • Other: Remove features

Which crates are being modified?

  • floresta-chain
  • floresta-cli
  • floresta-common
  • floresta-compact-filters
  • floresta-electrum
  • floresta-watch-only
  • floresta-wire
  • floresta
  • florestad
  • Other:

Description

The last usage of the florestad/experimental-p2p feature was removed at #209. It is not used anywhere now.

Also we have a floresta-wire/pruned_utreexo_chainstate feature which is not used either. I guess we could reintroduce it if in the future we have a non pruned chain (but maybe it is not needed since we work with trait bounds).

Notes to the reviewers

Our CI will also be faster after this, since the feature matrix grows exponentially with each feature. Particularly we halve the florestad compilations from 48 to 24.

@JoseSK999
Remove unused florestad and wire features
30c5bb9 
The last usage of the `florestad/experimental-p2p` feature was removed at vinteumorg#209. It is not used anywhere now.

Also we have a `floresta-wire/pruned_utreexo_chainstate` feature which is not used either. I guess we could reintroduce it if in the future we have a non pruned chain (but maybe it is not needed since we work with trait bounds).
@Davidson-Souza Davidson-Souza added dependencies Pull requests that update a dependency file chore Cleaning, refactoring, reducing complexity labels May 16, 2025
@jaoleal
Copy link
Collaborator

jaoleal commented May 16, 2025 •
edited
Loading

ack 30c5bb9

@lucad70
Copy link
Contributor

lucad70 commented May 16, 2025

ACK 30c5bb9

When and how was experimental-p2p used? Was it during the implementation of the p2p? And after it was implemented, it was deprecated?

Davidson-Souza
Davidson-Souza approved these changes May 16, 2025
View reviewed changes
Copy link
Member

@Davidson-Souza Davidson-Souza left a comment

Choose a reason for hiding this comment

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

ACK 30c5bb9

@Davidson-Souza
Copy link
Member

Davidson-Souza commented May 16, 2025 •
edited
Loading

When and how was experimental-p2p used? Was it during the implementation of the p2p? And after it was implemented, it was deprecated?

Yes, before having a p2p stack we did a client-server thing. The code is still there, but didn't keep up with the API changes after #58. Perhaps it could be removed all together?

@Davidson-Souza Davidson-Souza merged commit 8982623 into vinteumorg:master May 16, 2025
10 checks passed
@lucad70
Copy link
Contributor

lucad70 commented May 16, 2025

When and how was experimental-p2p used? Was it during the implementation of the p2p? And after it was implemented, it was deprecated?

Yes, before having a p2p stack we did a client-server thing. The code is still there, but didn't keep up with the API changes after #58. Perhaps it could be removed all together?

Yes, I believe we could remove unused code as well, to avoid having a codebase full of dead code in the future. Or, signal it as deprecated?

@JoseSK999 JoseSK999 deleted the remove-unused-features branch May 16, 2025 18:25
@JoseSK999
Copy link
Contributor Author

I guess it can be confusing to find a cli_wire module in floresta-wire that is just a fossil @Davidson-Souza

@luisschwab
Copy link
Contributor

I say send it to the void. I don't see why anyone would be using it.

@Davidson-Souza
Copy link
Member

Yeah. Since you can make your own wire if you need something like this, I guess there's not much use into keeping this weird stuff that will be mostly unused. From my side I concept ACK removing it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Davidson-Souza Davidson-Souza Davidson-Souza approved these changes

Assignees

No one assigned

Labels

chore Cleaning, refactoring, reducing complexity dependencies Pull requests that update a dependency file

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@JoseSK999 @jaoleal @lucad70 @Davidson-Souza @luisschwab