CChainState -> Chainstate by jamesob · Pull Request #24513 · bitcoin/bitcoin

jamesob · 2022-03-09T17:56:35Z

Alright alright alright, I know: we hate refactors. We especially hate cosmetic refactors.

Nobody knows better than I that changing broad swaths of code out from under our already-abused collaborators, only to send a cascade of rebase bankruptcies, is annoying at best and sadistic at worst. And for a rename! The indignation!

But just for a second, imagine yourself. Programming bitcoin/bitcoin, on a sandy beach beneath a lapis lazuli sky. You go to type the name of what is probably the most commonly used data structure in the codebase, and you only hit shift once.

What could you do in such a world? You could do anything. The only limit is yourself.

So maybe you like the idea of this patch but really don't want to deal with rebasing. You're in luck!

Here're the commands that will bail you out of rebase bankruptcy:

git rebase -i $(git merge-base HEAD master) \
  -x 'sed -i "s/CChainState/Chainstate/g" $(git ls-files | grep -E ".*\.(py|cpp|h)$") && git commit --amend --no-edit'
# <commit changed?>
git add -u && git rebase --continue

~~Anyway I'm not sure how serious I am about this, but I figured it was worth proposing.~~ I have decided I am very serious about this.

Maybe we can have nice things every once in a while?

jonatack · 2022-03-09T18:24:14Z

Ok, where is @jamesob and what have you done to him.

DrahtBot · 2022-03-11T06:45:08Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25977 (refactor: Replace std::optional<bilingual_str> with util::Result by ryanofsky)
#25971 (refactor: Use std::string for thread and index names by stickies-v)
#25740 (assumeutxo: background validation completion by jamesob)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
#25667 (assumeutxo: snapshot initialization by jamesob)
#25665 (refactor: Add util::Result failure values, multiple error and warning messages by ryanofsky)
#24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)
#23599 ([WIP] DRAFT NOMERGE Tidy up RPCTxSerializationFlags by MarcoFalke)
#23319 (rpc: Return fee and prevout (utxos) to getrawtransaction by dougEfresh)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

glozow · 2022-03-11T17:18:39Z

Concept ACK, but need song-and-dance interpretation of the PR description

jonatack · 2022-03-13T17:16:31Z

I'm sort of tepid on whether this is worth doing, but supposing it were, I think I'd prefer ChainState over Chainstate. The reason/question is, are expressions/words like "chain state", "fee rate", and "block hash" each one word or two? For me, and maybe for people not closely familiar since years with Bitcoin development, they are two words--at least originally, which I guess is why there are classes in the codebase like CChainState and CFeeRate. I usually grep case-indifferently anyway, so this wouldn't change anything (if it doesn't break anything). Just one weakly held light opinion.

jb55 · 2022-03-13T23:24:07Z

ACK 5977234384da1f52475fc8165c52830750bf7d2a

michaelfolkson · 2022-03-15T14:12:18Z

I was reading through some of @jonatack's notes and found this old quote from Greg Maxwell:

I reiterate: the background level of refactors, style changes, cleanups, and other related activity is actively repelling multiple long term contributors, myself included. I beg: give it a bit of a rest, we have so many other things that are crying for our attention and our resolve. We should try to find some initiatives that we all feel more excited about, success with them would make it easier to work on other things... but right now a big style change is just not a unifying effort.

I don't have a strong enough opinion to NACK this PR but the above seems to add some historical context behind previous attempts to do renames like this which may be helpful for other reviewers.

ajtowns · 2022-03-15T14:22:51Z

Introducing "Stockholm syndrome: the PR" ? We have ChainstateManager already, so while I agree with @jonatack that it's weird treating "chainstate" as one word, at least this is slightly reducing an inconsistency. Don't really see the benefits outweighing the costs here.

ryanofsky

Code review ACK 5977234384da1f52475fc8165c52830750bf7d2a

https://www.youtube.com/watch?v=j7OHG7tHrNM

ryanofsky

Code review ACK e2c9115af2db4b5aa1f1d2333946fb22912e500d. Just rebased since last review

jamesob · 2022-09-07T19:52:53Z

maflcko · 2022-09-08T06:09:23Z

I sometimes see myself running into compile errors when typing ChainStateManager, or CChainstate, so I think it makes sense to somehow improve the consistency here. Also, the merge conflicts right now are currently as minimal as they can get.

Any reason why there are two commits and not a single one? A sed ... $(git grep -l CChainState ':(exclude)doc/release-notes*') should be sufficient.

-BEGIN VERIFY SCRIPT- sed -i 's/CChainState/Chainstate/g' $(git grep -l CChainState ':(exclude)doc/release-notes*') -END VERIFY SCRIPT- Co-authored-by: MacroFake <falke.marco@gmail.com>

jamesob · 2022-09-09T15:50:59Z

@MarcoFalke thanks! Pushed.

maflcko · 2022-09-10T08:39:18Z

cr ACK 00eeb31

hebasto · 2022-09-10T10:47:03Z

Concept ACK.

hebasto

ACK 00eeb31

glozow

ACK 00eeb31, thanks for being the one to propose this

w0xlt

ACK 00eeb31

jamesob · 2022-09-12T20:39:38Z

RFM?

Summary: ``` -BEGIN VERIFY SCRIPT- sed -i 's/CChainState/Chainstate/g' $(git grep -l CChainState ':(exclude)doc/release-notes*') arc lint -END VERIFY SCRIPT- ``` Co-authored-by: MacroFake <falke.marco@gmail.com> This is a backport of [[bitcoin/bitcoin#24513 | core#24513]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Differential Revision: https://reviews.bitcoinabc.org/D13126

jamesob force-pushed the 2022-03-chainstate-rename branch 4 times, most recently from 003d4af to 5977234 Compare March 9, 2022 18:16

DrahtBot added Block storage Mempool Mining RPC/REST/ZMQ Utils/log/libs UTXO Db and Indexes Validation labels Mar 9, 2022

DrahtBot mentioned this pull request Mar 11, 2022

Only load BlockMan in BlockMan member functions #24515

Merged

ryanofsky approved these changes Mar 15, 2022

View reviewed changes

This was referenced Jun 24, 2022

mempool: Add option to bypass contextual timelocks in testmempoolaccept #25434

Closed

Add a -mempoolfullrbf node setting #25353

Merged

refactor: Reduce number of LoadChainstate parameters and return values #25308

Merged

ryanofsky approved these changes Jun 28, 2022

View reviewed changes

DrahtBot mentioned this pull request Jun 29, 2022

indexes: Stop using node internal types #25494

Merged

DrahtBot added the Needs rebase label Jun 29, 2022

jamesob force-pushed the 2022-03-chainstate-rename branch from e2c9115 to 30ee3d3 Compare September 7, 2022 19:50

DrahtBot removed the Needs rebase label Sep 7, 2022

This was referenced Sep 8, 2022

refactor: Replace std::optional<bilingual_str> with util::Result #25977

Merged

refactor: Use std::string for thread and index names #25971

Merged

scripted-diff: rename CChainState -> Chainstate

00eeb31

-BEGIN VERIFY SCRIPT- sed -i 's/CChainState/Chainstate/g' $(git grep -l CChainState ':(exclude)doc/release-notes*') -END VERIFY SCRIPT- Co-authored-by: MacroFake <falke.marco@gmail.com>

jamesob force-pushed the 2022-03-chainstate-rename branch from 30ee3d3 to 00eeb31 Compare September 9, 2022 15:50

hebasto approved these changes Sep 10, 2022

View reviewed changes

glozow reviewed Sep 12, 2022

View reviewed changes

w0xlt approved these changes Sep 12, 2022

View reviewed changes

glozow merged commit 3a7e0a2 into bitcoin:master Sep 13, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 13, 2022

Merge bitcoin#24513: CChainState -> Chainstate

be7ad88

satsie mentioned this pull request Oct 7, 2022

Chapter 3 - Add link to PR that renames CChainState chaincodelabs/onboarding-to-bitcoin-core#45

Merged

bitcoin locked and limited conversation to collaborators Sep 13, 2023

Uh oh!

Conversation

jamesob commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Mar 9, 2022

Uh oh!

DrahtBot commented Mar 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

glozow commented Mar 11, 2022

Uh oh!

jonatack commented Mar 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jb55 commented Mar 13, 2022

Uh oh!

michaelfolkson commented Mar 15, 2022

Uh oh!

ajtowns commented Mar 15, 2022

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

jamesob commented Sep 7, 2022

Uh oh!

maflcko commented Sep 8, 2022

Uh oh!

jamesob commented Sep 9, 2022

Uh oh!

maflcko commented Sep 10, 2022

Uh oh!

hebasto commented Sep 10, 2022

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

jamesob commented Sep 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

jamesob commented Mar 9, 2022 •

edited

Loading

DrahtBot commented Mar 11, 2022 •

edited

Loading

jonatack commented Mar 13, 2022 •

edited

Loading