AlreadyAskedFor -> WaitingFor rename, for reasons of least surprise. #1341
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Also, reduces confusion when combined with future pull requests.
|
I know from own observations, that core devs dislike variable or function renaming without a very good reason ;), only wanted to say... |
Contributor
Author
|
Principle of least astonishment is usually the best reason for such a change, along with naming conventions probably coming a close second (or joint first). |
Contributor
|
NAK, breaks other more important changes for little gain |
suprnurd
pushed a commit
to chaincoin-legacy/chaincoin
that referenced
this pull request
Dec 5, 2017
* Few networking fixes: - skip "masternode"/inbound connections for sync related processes - do not sync gov data to other nodes until fully synced ourselves - do not accept incoming connections until fully synced * inbound connections could be harmful only if our node is a masternode * same for CGovernanceManager::Sync
lateminer
pushed a commit
to lateminer/bitcoin
that referenced
this pull request
Jan 22, 2019
Add missing debug categories debug helper message
lateminer
pushed a commit
to lateminer/bitcoin
that referenced
this pull request
May 6, 2020
64465b7 [Refactor] Consensus: Stake Modifier V2 (random-zebra) 8083725 [Refactor] Consensus: TimeProtocolV2 parameters (random-zebra) 4d1c696 [Refactor] Consensus: nStakeMinAge, nStakeMinDepth, nFutureTimeDrift (random-zebra) e69cee2 [Refactor] Consensus: restore nMaxMoneyOut as consensus param (random-zebra) 08a581d [Refactor] Consensus: fAllowMinDifficultyBlocks, fPowNoRetargeting (random-zebra) ca6ea3c [Refactor] Consensus: nTargetTimespan, nTargetSpacing, nTimeSlotLength (random-zebra) a281dee [Refactor] Consensus: remove unneeded getters. Rename BIP65Height (random-zebra) 284271c [Refactor] Consensus: bnProofOfWorkLimit / bnProofOfStakeLimit (v1-v2) (random-zebra) Pull request description: This PR builds on - [x] bitcoin#1328 Populates the `consensus` struct with all relevant consensus parameters. Resets `nMaxMoneyOut` as chain param (with different value for TestNet). Cleans up several unneeded fields in ChainParams. EDIT: Current PR has been split in two. Second part is bitcoin#1344 ACKs for top commit: furszy: good, utACK 64465b7. Fuzzbawls: utACK 64465b7 Tree-SHA512: d3e46bc8bb2ad9ee48abcbc4d4b1df2f59914360aaca72e93e19a5578248ecb95fd6a422477238e4736bc767d4e226f5b7f867174d96dcace73621946bf84df8
lateminer
pushed a commit
to lateminer/bitcoin
that referenced
this pull request
May 6, 2020
92221b1 [Cleanup] Remove ChainParams::DefaultMinerThreads (random-zebra) 98494ab [Refactor] Consensus: move other consensus params to the struct (random-zebra) 7c673bf [Refactor] Consensus: masternode/budget-related fields (random-zebra) 7736d6c [Refactor] Consensus: zerocoin-related fields (random-zebra) 0b63af4 [Refactor] ChainParams: move genesis creation at the beginning (random-zebra) 1e5a33d [Refactor] Consensus: wrapped serials params (nSupplyBeforeFakeSerial) (random-zebra) 70c4ae5 [Refactor] Consensus: height-based activations (random-zebra) Pull request description: Second part of - [x] bitcoin#1341 Split as individual PR for ease of review. ACKs for top commit: Fuzzbawls: ACK 92221b1 furszy: utACK 92221b1. Tree-SHA512: a3f1c10fc568e264039d2fc5a4dedc01d947b380e55c5170679598cb6859274c73157fb9ade6f5345a9781b9028943629b304aac8c3b0d7849946bdaed85fb52
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Also, reduces confusion when combined with future pull requests.
This is a small change, and possibly seems petty, but it's useful for two reasons:
Principle of least surprise. Waiting can end (and does in the code), but Already Asked for requires a past event that's happened to unhappen. This is confusing.
Later code (which I am hoping gets pulled when finished, such as Parallel block download #1326) uses variables that make sense, and makes the code simpler to understand when used with this renamed mapping.
probably other reasons....