Skip to content

Implement pruneblock method and test #132

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.

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

Merged
merged 1 commit into from
May 5, 2025

Conversation

GideonBature
Copy link
Contributor

@GideonBature GideonBature commented Apr 23, 2025

Going by the conversations with tcharding for PR #116 on implementing one method at a time for easier review: This is the first method implementation pruneblockchain which is a specific type that returns a standard type (numeric). Once this is approved, I’ll proceed with the remaining ones.

@GideonBature GideonBature force-pushed the pruneblockchain branch 2 times, most recently from 371f56c to 2d05895 Compare April 23, 2025 11:04
Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

Looks pretty good man. Can you strip out all the unrelated changes then I'll do another review. Keep at it man, you're killing it.

@tcharding tcharding marked this pull request as draft April 29, 2025 10:26
@tcharding
Copy link
Member

Please undraft only after you've seen to all previous review suggestions. Thanks

@GideonBature GideonBature marked this pull request as ready for review April 29, 2025 17:16
@tcharding
Copy link
Member

Still has formatting changes in it.

@tcharding
Copy link
Member

Hey mate, I just merged #117, I've been working on that for a while now. As such it has some of my latest ideas in it. In general if you want to copy something you might want to reach into the raw_transactions modules first.

@GideonBature
Copy link
Contributor Author

Hey mate, I just merged #117, I've been working on that for a while now. As such it has some of my latest ideas in it. In general if you want to copy something you might want to reach into the raw_transactions modules first.

Alright, I am going to do that... Thank you. The work you did is massive.

@tcharding
Copy link
Member

Please squash the commits into a single one.

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

Looking good, couple more things. Stick at it!

@GideonBature GideonBature force-pushed the pruneblockchain branch 8 times, most recently from 29760bb to a1a308e Compare April 30, 2025 15:55
@GideonBature
Copy link
Contributor Author

I am having a hard time squashing the commits, the one having the merge after resolving conflict seems to not appear for me to squash to the original one that was there, so I want to ask if there is a way to resolve that, or If I should close this PR and open a fresh PR with the final stage of this PR?

@GideonBature GideonBature marked this pull request as draft April 30, 2025 16:55
@tcharding
Copy link
Member

Yeah something strange has happened to the git index on this branch. I managed to get it kind of sane again using the following series of commands.

  • git reset --hard @~
  • git clean -fd

Then I was able to run git rebase master. This leaves a bunch of conflicts in types. If I was you I'd just hard reset types and re-do it manually but that is just my opinion.

@GideonBature
Copy link
Contributor Author

Yeah something strange has happened to the git index on this branch. I managed to get it kind of sane again using the following series of commands.

  • git reset --hard @~
  • git clean -fd

Then I was able to run git rebase master. This leaves a bunch of conflicts in types. If I was you I'd just hard reset types and re-do it manually but that is just my opinion.

Noted... I will try your opinion... Thank you.

@GideonBature GideonBature force-pushed the pruneblockchain branch 4 times, most recently from 8c00f0b to 4ceedaa Compare May 1, 2025 18:24
@GideonBature GideonBature marked this pull request as ready for review May 1, 2025 18:28
@GideonBature
Copy link
Contributor Author

Yeah something strange has happened to the git index on this branch. I managed to get it kind of sane again using the following series of commands.

  • git reset --hard @~
  • git clean -fd

Then I was able to run git rebase master. This leaves a bunch of conflicts in types. If I was you I'd just hard reset types and re-do it manually but that is just my opinion.

This worked... Thank you :).

@tcharding
Copy link
Member

Ugh, bro! Now you have changes from the v29 PR in there. Can you flick through your diffs before you push them to help catch these things.

@GideonBature GideonBature force-pushed the pruneblockchain branch 2 times, most recently from 19fd0b5 to 2e76667 Compare May 2, 2025 23:30
@tcharding
Copy link
Member

This is getting clean! One final thing, and I wouldn't normally ask but because this PR is a test run for more of the same then we should get it perfect. The integration test is not in order (should be below the blockchain__precious_block test). Then we can merge, well done man.

@GideonBature
Copy link
Contributor Author

This is getting clean! One final thing, and I wouldn't normally ask but because this PR is a test run for more of the same then we should get it perfect. The integration test is not in order (should be below the blockchain__precious_block test). Then we can merge, well done man.

smiles, honestly, I thought of the order too, but I thought best I hear it from you. Thank you so much for the reviews, sometimes I feel I am stressing you. I have learnt a lot from the reviews you gave me, and will make sure I do the remaining ones the right way.

@tcharding
Copy link
Member

tcharding commented May 5, 2025

No stress man, if you apply effort and learn from your mistakes then we all win.

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK bbcec24

@tcharding tcharding merged commit dc0122e into rust-bitcoin:master May 5, 2025
55 of 56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants