Comparing two empty CChains is currently undefined behaviour, and resulted in false assertion failures when comparing identical empty CChains in local testing.
Let's just remove this comparison operator since it doesn't seem to be used anywhere.
chain: Fix CChain comparison UB by removing it (it was unused) #19822
pull dongcarl wants to merge 1 commits into bitcoin:master from dongcarl:2020-08-chain-comparison-UB changing 1 files +0 −6-
dongcarl commented at 4:58 PM on August 27, 2020: member
- dongcarl added the label Bug on Aug 27, 2020
-
practicalswift commented at 6:16 PM on August 27, 2020: contributor
Oh, nice catch! This code was introduced back in 2013 (see 4c6d41b8b653ef90639b1a32f6aab0bb1cef90c5).
Fuzzing with the sanitizers would have trivially caught this one but it seems like we're missing fuzzing coverage for
CChain::operator==: https://marcofalke.github.io/btc_cov/fuzz.coverage/src/chain.h.gcov.htmlWill continue working on increasing fuzzing coverage as soon as the fuzzers that are currently under review have been processed (merged or rejected). If the review speed picks up a bit in
src/test/fuzz/I think we'll soon achieve fuzzing coverage on par with -- or exceeding -- the unit test coverage :) Review welcome! ❤️ACK 2c0e6403b6cca1c1ed42a03c14912ad4a03f277f -- diff looks correct, but I prefer deleting broken unused functions instead of fixing them: deleting them removes the need to reason about the correctness of the fix entirely :)
-
jamesob commented at 8:49 PM on August 27, 2020: member
-
sipa commented at 9:24 PM on August 27, 2020: member
If it's unused, just delete it.
-
df536883d2
chain: Remove UB CChain comparison
It was unused, and had UB
- dongcarl force-pushed on Aug 28, 2020
- dongcarl renamed this:
chain: Fix CChain comparison UB
chain: Fix CChain comparison UB by removing it (it was unused)
on Aug 28, 2020 -
practicalswift commented at 5:13 AM on August 28, 2020: contributor
ACK df536883d263781c2abe944afc85f681cda635ed -- patch is guaranteed to be correct :)
-
MarcoFalke commented at 5:52 AM on August 28, 2020: member
cr ACK df536883d263781c2abe944afc85f681cda635ed
I wonder why #19702 doesn't detect this
- fanquake merged this on Aug 28, 2020
- fanquake closed this on Aug 28, 2020
- PastaPastaPasta referenced this in commit 7e1e91dab3 on Jun 27, 2021
- PastaPastaPasta referenced this in commit cea7016cf2 on Jun 28, 2021
- PastaPastaPasta referenced this in commit 85f3280068 on Jun 29, 2021
- PastaPastaPasta referenced this in commit 964e31ac85 on Jul 1, 2021
- PastaPastaPasta referenced this in commit c73eb3398b on Jul 1, 2021
- PastaPastaPasta referenced this in commit 5271ac6bae on Jul 15, 2021
- PastaPastaPasta referenced this in commit f742025444 on Jul 16, 2021
- Fabcien referenced this in commit ba848a43e2 on Sep 16, 2021
- DrahtBot locked this on Feb 15, 2022
Contributors
Labels