Fix typos #28605

Sjors commented at 7:48 AM on October 6, 2023: member

This PR fixes typos found by lint-spelling.py using codespell 2.2.6.

Our CI linter job uses codespell 2.2.5 and found fewer typos that I did locally. In any case it's happy now.

DrahtBot commented at 7:49 AM on October 6, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	pablomartin4btc
Concept NACK	fanquake, maflcko
Stale ACK	hebasto, jarolrod, hernanmarino, achow101, josibake

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28710 (Remove the legacy wallet and BDB dependency by achow101)
#27788 (rpc: Be able to access RPC parameters by name by achow101)
#25273 (wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction by achow101)

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.

in test/lint/spelling.ignore-words.txt:9 in b0ed6e5d26 outdated

   5 | @@ -6,6 +6,7 @@ bu
   6 |  cachable
   7 |  clen
   8 |  crypted
   9 | +debbugs

Sjors commented at 7:50 AM on October 6, 2023:

This is a reference to a Github repo in the Guix install doc.

in src/minisketch/src/sketch_impl.h:220 in b0ed6e5d26 outdated

 216 | @@ -217,7 +217,7 @@ bool RecFindRoots(std::vector<std::vector<typename F::Elem>>& stack, size_t pos,
 217 |          }
 218 |  
 219 |          if (fully_factorizable) {
 220 | -            // Every succesful iteration of this algorithm splits the input
 221 | +            // Every successful iteration of this algorithm splits the input

maflcko commented at 7:58 AM on October 6, 2023:

You'll have to submit/mirror this upstream, no?

Sjors commented at 8:01 AM on October 6, 2023:

Or have the linter skip it. I'll take a look.

Sjors commented at 8:02 AM on October 6, 2023:

Ah it already does that. Will undo.

Sjors force-pushed on Oct 6, 2023

DrahtBot added the label CI failed on Oct 6, 2023

Sjors commented at 8:00 AM on October 6, 2023: member

Updated to also fix re-use -> reuse (except in historical release notes, which the linter skips).

in src/test/fuzz/FuzzedDataProvider.h:161 in ad08990905 outdated

 157 | @@ -158,7 +158,7 @@ FuzzedDataProvider::ConsumeRandomLengthString(size_t max_length) {
 158 |    // picking its contents.
 159 |    std::string result;
 160 |  
 161 | -  // Reserve the anticipated capaticity to prevent several reallocations.
 162 | +  // Reserve the anticipated capacity to prevent several reallocations.

maflcko commented at 8:03 AM on October 6, 2023:

Thanks, fixed in https://github.com/llvm/llvm-project/commit/fad0919c508a666eb634ea9793a69322143dbd69

Sjors commented at 8:08 AM on October 6, 2023:

I'll leave that fix in then.

Sjors force-pushed on Oct 6, 2023

Sjors commented at 8:09 AM on October 6, 2023: member

Dropped typo fix in sketch_impl.h and added a warning to the release process to not (accidentally) touch typos in upstream code.

DrahtBot removed the label CI failed on Oct 6, 2023

hernanmarino commented at 10:13 PM on October 6, 2023: contributor

This is a nice idea. I always have a conflict between submitting this simple typo-fixing PRs and ignoring them. This should be done often, for evey release .

We probably don't want too many pull requests that just fix a typo. At the same time, if we never fix them people will start ignore the linter all-together. So my suggestion would be to do this before branch-off.

hebasto commented at 11:42 AM on October 7, 2023: member

Our CI linter job uses codespell 2.2.5 and found fewer typos that I did locally. In any case it's happy now.

Why not bumping the codespell version in CI simultaneously?

hebasto approved

hebasto commented at 12:09 PM on October 7, 2023: member

I reckon that changes in such a pull request should be forced by a linter. However, with the picked change into the spelling.ignore-words.txt file, the linter output is as follows:

$ ./test/lint/lint-spelling.py 
src/addrman.h:175: occurence ==> occurrence
src/net_processing.cpp:2450: annnounced ==> announced
src/net_processing.cpp:3595: succesful ==> successful
src/policy/policy.h:101: compatability ==> compatibility
src/qt/transactiontablemodel.cpp:222: re-use ==> reuse
src/rpc/server.cpp:412: unspecifed ==> unspecified
src/rpc/util.cpp:1091: re-use ==> reuse
src/rpc/util.h:409: documention ==> documentation
src/test/fuzz/FuzzedDataProvider.h:161: capaticity ==> capacity
src/test/orphanage_tests.cpp:114: Re-use ==> Reuse
src/test/script_tests.cpp:1115: re-use ==> reuse
src/torcontrol.cpp:254: implementors ==> implementers
src/txmempool.h:480: appplies ==> applies
src/wallet/feebumper.cpp:173: re-use ==> reuse
src/wallet/spend.cpp:1260: re-use ==> reuse
src/wallet/spend.cpp:1302: Re-use ==> Reuse
test/functional/test_framework/blockfilter.py:32: relvant ==> relevant
test/functional/wallet_avoidreuse.py:260: re-use ==> reuse
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt

Apparently, the linter ignores the word "re-used" in the following files:

ci/README.md
depends/description.md
src/test/versionbits_tests.cpp
src/wallet/wallet.h

The overall changes look consistent though.

ACK 650e22d65bd3504f51547f4d70d6d74101ff602f.

Sjors commented at 10:50 AM on October 8, 2023: member

Why not bumping the codespell version in CI simultaneously?

I'll look into that if I have to retouch. At least we know that such a bump won't reveal new typos, so it can be done safely anytime.

jarolrod commented at 4:23 AM on October 12, 2023: member

ACK 650e22d65bd3504f51547f4d70d6d74101ff602f

DrahtBot requested review from hebasto on Oct 12, 2023

DrahtBot removed review request from hebasto on Oct 12, 2023

hernanmarino approved

hernanmarino commented at 2:36 PM on October 16, 2023: contributor

ACK 650e22d65bd3504f51547f4d70d6d74101ff602f

in src/torcontrol.cpp:254 in 650e22d65b outdated

 250 | @@ -251,7 +251,7 @@ std::map<std::string,std::string> ParseTorReplyMapping(const std::string &s)
 251 |              /**
 252 |               * Unescape value. Per https://spec.torproject.org/control-spec section 2.1.1:
 253 |               *
 254 | -             *   For future-proofing, controller implementors MAY use the following
 255 | +             *   For future-proofing, controller implementers MAY use the following

pablomartin4btc commented at 2:49 PM on October 16, 2023:

Actually, both implementors & implementers are valid words, but it seems they have different meaning (Implementer: One who has implemented, Implementor: One who provides implementation).

pablomartin4btc commented at 2:51 PM on October 16, 2023: member

ACK 650e22d65bd3504f51547f4d70d6d74101ff602f

achow101 commented at 5:48 PM on October 16, 2023: member

ACK 650e22d65bd3504f51547f4d70d6d74101ff602f

fanquake commented at 5:50 PM on October 16, 2023: member

NACK on adding this to the release process. I don't understand why the CI infra isn't being changed here. This is the wrong solution to a non-problem.

fanquake commented at 1:29 PM on October 17, 2023: member

To clarify:

If the lint infra doesn't work (is ignored by everyone, and doesn't enforce the linting via failure) then it should be just removed. Otherwise, why do we have it? It basically just results in random PRs to "fix"/bump things.
If we are going to keep the infra, and change anything here, why wouldn't it be updated to the newest version available, and all issues fixed?
Adding anything like this to the release process is just reinforcement of the lint infra being pointless:
- You are just delegating to someone (probably a maintainer) to run codespell pre branch off.
- Trivial things should not be added to an (already messy) doc that contains important steps for creating a release.

so it can be done safely anytime.

We aren't changing consensus code here, why would it be unsafe to fix typos at any time?

josibake commented at 2:09 PM on October 17, 2023: member

Also a NACK on adding this to the release process. We should be looking for ways to reduce manual steps and further automate the release process. The last commit adds a manual step and doesn't address the root cause of how these spelling errors are getting merged in the first place.

I'd suggest we drop https://github.com/bitcoin/bitcoin/pull/28605/commits/650e22d65bd3504f51547f4d70d6d74101ff602f and instead, look into updating the linting infrastructure so that these errors are caught going forward. If after fixing the linting infrastructure we still need manual fix up PRs, they should happen whenever someone is sufficiently motivated to do it, and not be tied to a release process. Ideally, any future fix-up PRs should also include updates to the linting infrastructure to prevent the same type of errors from sneaking through.

josibake commented at 2:10 PM on October 17, 2023: member

ACK https://github.com/bitcoin/bitcoin/pull/28605/commits/e062212720900ccd295c83b7e1753089980988b4

DrahtBot requested review from josibake on Oct 17, 2023

DrahtBot added the label CI failed on Oct 22, 2023

Sjors commented at 8:21 AM on October 23, 2023: member

My understanding is that typos used to cause builds to fail, but that was consider too annoying.

(1) Happy to change this PR to make typos cause a fail.

Otherwise we should either have: (2) a process to fix them (this PR)

or (3) ignore them all together.

But even if we decide to ignore typos (3) there will always be someone making a PR to fix the typos they find. So imo only (1) and (2) make sense.

Sjors commented at 8:28 AM on October 23, 2023: member

See earlier discussion about warning vs error in #13954 and #14495. Best quote:

please let's not go back and forth with this or i'm going to add a linter that rejects linters

maflcko commented at 8:54 AM on October 23, 2023: member

(1) Happy to change this PR to make typos cause a fail.

NACK. This may cause silent merge conflicts (red CI), even if an author doesn't change the pull request at all. For example, when bumping the spellchecker version. This is problematic, because bumping this spellchecker usually comes with having to fight false positives, which doesn't seem worth it to load off on to pull request authors. Also, silent merge conflicts could mean that a reviewed pull request (ready for merge) has a failing CI shortly before merge, or worse, after merge. Finally, even absent the above issues, loading off the false positive fighting to pull request authors is not a good use of their time.

unrelated: It could make sense to document this in the linter code, to avoid having to re-hash the discussion every two years.

No opinion on whether to add the false positive fighting to the release process. Whether or not it is added, no one is holding anyone back from fixing typos before the branch-off of a release.

josibake commented at 3:30 PM on October 24, 2023: member

Agree that our position on linting should be documented, ideally in the contributing docs or developer notes.

(2) a process to fix them (this PR)

Opening a PR before branch off which fixes many typos in one go seems fine. I don't see any value in making it part of the release process (or even adding it as a suggestion to the release docs) for the following reasons:

It is not at all essential for cutting a release
There is no reason to wait for a release to open a bundled "bunch of typo fixes" PR
It has no benefit to the end user (the direct "customer" of the release process)

I'd suggest dropping the last commit unless there is something demonstrably wrong with the current process (fixing typos, ideally multiple fixes in one PR, whenever the need arises) and it's clear how suggesting this be done as part of a release addresses that problem.

Sjors commented at 7:04 AM on October 25, 2023: member

@josibake afaik there is no other documented process besides the release process. Waiting for something to randomly get done by someone is not a process, and tends to lead to either:

nobody doing it: this leads to exponential growth in typos because fewer people will look at the linter output for each PR; or
too many people do it, and too often, since it's an easy first contribution. At that point they would ask "when is a good time?", this provides an answer.

It says "Consider" to make clear it's not required for every release. There are other non-mandatory steps in that document.

Translations happen every release, so it's good to get typos in translated strings out of the way. Otherwise, if you fix a typo after a string has been translated, the UI will fall back to the English string (I think, cc @hebasto). Not all typos are in strings that get translated, but might as well do it at the same time.

maflcko commented at 7:42 AM on October 25, 2023: member

tends to lead to either:

1. nobody doing it: this leads to exponential growth

2. too many people do it, and too often, since it's an easy first contribution.

Neither is true, looking at how often they were fixed in the past, see https://github.com/bitcoin/bitcoin/pulls?q=is%3Apr+is%3Aclosed+codespell+. It seemed to have worked just fine in the past, so I don't see the need to change it? Also, I don't understand why this should be disallowed to be an easy first contribution. If this helps someone to get started with the local and GitHub development workflow, and they stick around in the project, providing useful input in the long run. I don't see the downside.

Obviously, if it is done never, or too often, the process can be changed, but absent that it seems better to leave it as-is.

Sjors commented at 6:15 AM on October 26, 2023: member

If you search for "fix typo" pull requests there's hundreds: https://github.com/bitcoin/bitcoin/pulls?q=is%3Apr+is%3Aclosed+%22fix+typo%22

And there's confusion about whether or not they're desirable: #27268 (comment)

There's also confusion about how to deal with upstream docs (e.g. #28022). That can be clarified in the developer docs, but I suspect some reviewers will overlook that.

Anyway, I guess the question is for the maintainers. Feel free drop the last commit when merging (or I can do it).

maflcko commented at 7:02 AM on October 26, 2023: member

If you search for "fix typo" pull requests there's hundreds:

This has nothing to do with lint-spelling.py/codespell, because codespell doesn't find any of those typos, see also #28624#pullrequestreview-1668665734

DrahtBot removed the label CI failed on Oct 27, 2023

maflcko commented at 9:34 AM on November 6, 2023: member

Are you still working on this? Otherwise, I think it can be closed, so that it can be picked up by someone else, if it is still relevant.

Sjors commented at 9:43 AM on November 6, 2023: member

I was still waiting for feedback from maintainers, but I'll just drop the last commit.

Sjors renamed this:
~~Fix typos and suggest doing so before branch-off~~
Fix typos
on Nov 6, 2023

Sjors force-pushed on Nov 6, 2023

DrahtBot removed review request from josibake on Nov 6, 2023

DrahtBot requested review from hebasto on Nov 6, 2023

DrahtBot requested review from hernanmarino on Nov 6, 2023

DrahtBot requested review from pablomartin4btc on Nov 6, 2023

DrahtBot requested review from achow101 on Nov 6, 2023

DrahtBot requested review from jarolrod on Nov 6, 2023

fanquake commented at 9:56 AM on November 6, 2023: member

test/functional/feature_assumeutxo.py:78: refering ==> referring
test/functional/feature_assumeutxo.py:115: refering ==> referring
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt

DrahtBot removed review request from hernanmarino on Nov 6, 2023

DrahtBot requested review from hernanmarino on Nov 6, 2023

maflcko commented at 10:17 AM on November 6, 2023: member

I was still waiting for feedback from maintainers

DrahtBot has a summary comment, which has links to all feedback, including that of maintainers, see #28605 (comment)

DrahtBot removed review request from hernanmarino on Nov 6, 2023

DrahtBot requested review from hernanmarino on Nov 6, 2023

doc: fix typos

As found by lint-spelling.py using codespell 2.2.6.

43de4d3630

Sjors force-pushed on Nov 7, 2023

Sjors commented at 1:22 AM on November 7, 2023: member

Rebased to fix those two new typos.

pablomartin4btc approved

pablomartin4btc commented at 1:46 AM on November 7, 2023: member

re ACK 43de4d3630274e1287179c86896ed4c2d8b9eff4

DrahtBot removed review request from hernanmarino on Nov 7, 2023

DrahtBot requested review from josibake on Nov 7, 2023

DrahtBot requested review from hernanmarino on Nov 7, 2023

fanquake commented at 10:35 AM on November 16, 2023: member

There are even more since this was last updated, but going to merge this now, and we can follow up again later.

DrahtBot removed review request from hernanmarino on Nov 16, 2023

DrahtBot requested review from hernanmarino on Nov 16, 2023

fanquake merged this on Nov 16, 2023

fanquake closed this on Nov 16, 2023

Sjors deleted the branch on Nov 17, 2023

bitcoin locked this on Nov 19, 2024