Skip to content
New issue

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

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

Already on GitHub? # to your account

Specify whether bootstrap pull should start at block hash or account #4018

Merged

Conversation

pwojcikdev
Copy link
Contributor

@pwojcikdev pwojcikdev commented Dec 5, 2022

There are accounts with the same hash as another block in our live ledger. That creates ambiguity and makes it impossible to pull those account chains. This PR removes that ambiguity by always specifying whether requested hash is a block or account.

@clemahieu
Copy link
Contributor

Is this something that would be better as a header flag?

@pwojcikdev pwojcikdev force-pushed the prs/bootstrap-server-start-type branch from 3e143ac to 7c95789 Compare December 7, 2022 20:41
@dsiganos
Copy link
Contributor

dsiganos commented Dec 7, 2022

I do not agree with creating an enum for something that has only 2 states.
I would prefer to see a flag used. But other than that, looks good to me.

@pwojcikdev
Copy link
Contributor Author

@dsiganos I don't understand how enum conflicts with using flags? IMHO it's much better code style to define you flags as an enum (in contrast to defining const ints, C style). One of the first links I found on the internet you might want to check out, specifically the section on std::bitset https://m-peko.github.io/craft-cpp/posts/different-ways-to-define-binary-flags/

@pwojcikdev pwojcikdev merged commit 7965c51 into nanocurrency:develop Dec 8, 2022
@pwojcikdev pwojcikdev deleted the prs/bootstrap-server-start-type branch December 8, 2022 14:23
@dsiganos
Copy link
Contributor

dsiganos commented Dec 8, 2022

@pwojcikdev This is mostly stylistic disagreement so no big deal if we go one way or another.
I come from a traditional C background so uint8 with masks, seems perfectly natural to me.
For me, the fact that the enum has more than 2 states causes an unwanted complication.
But, this is just a stylistic disagreement, so it doesn't really matter.

clemahieu pushed a commit that referenced this pull request Dec 9, 2022
…4018)

* Specify whether pull should start at block hash or account

* Specify account info request target type
@thsfs thsfs added bug and removed enhancement labels May 22, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants