refactor: Replace &foo[0] with foo.data()

MarcoFalke commented at 6:24 pm on April 30, 2021: member

The main theme of this refactor is to replace &foo[0] with foo.data().

The first commit is taken from #21781 with the rationale:

In CSignatureCache::ComputeEntryECDSA, change the way a vector pointer is resolved to prevent invoking undefined behavior if the vector is empty.

The other commits aim to remove all &foo[0], where foo is any kind of byte representation. The rationale:

Sometimes alternative code without any raw data pointers is easier to read (refer to the respective commit message for details)
If the raw data pointer is needed, foo.data() should be preferred, as pointed out in the developer notes. This addresses the instances that have been missed in commit 592404f03f2b734351d734f0c9ca1fdce997321b, and https://github.com/bitcoin/bitcoin/pull/9804

MarcoFalke added the label Refactoring on Apr 30, 2021

practicalswift commented at 8:54 pm on April 30, 2021: contributor

Concept ACK

&foo[0] does not spark joy.

foo.data() sparks joy (or at least more joy than &foo[0]).

DrahtBot commented at 1:50 am on May 1, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21781 (Security enhancements to ChaCha20::SetKey and CSignatureCache::ComputeEntryECDSA by guidovranken)
#15294 ([moveonly] wallet: Extract RipeMd160 by Empact)

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.

MarcoFalke commented at 6:37 am on May 1, 2021: member

Fun facts:

Commit fabb6dfe6e doesn’t change the bitcoind binary on -O2 with g++ and clang++ on my system
Same for faece47c47
Same for fac30eec42 [1]

So the only commits that changes bitcoind are the Span refactor and the dbwrapper refactor.

[1] Edit: For g++ I get a small diff:

 0diff --git a/tmp/old/d_objdump b/tmp/new/d_objdump
 1index 2b7eabdab2..244eabc8de 100644
 2--- a/tmp/old/d_objdump
 3+++ b/tmp/new/d_objdump
 4@@ -871846,7 +871846,7 @@ Disassembly of section .text:
 5   3f7234:      cmp    $0x3,%eax
 6   3f7237:      ja     3f725a <LegacyScriptPubKeyMan::IsMine(CScript const&) const+0x4a>
 7   3f7239:      mov    %eax,%eax
 8-  3f723b:      lea    0x28389e(%rip),%rdx        # 67aae0 <CSWTCH.5100>
 9+  3f723b:      lea    0x28389e(%rip),%rdx        # 67aae0 <CSWTCH.5101>
10   3f7242:      mov    (%rdx,%rax,4),%eax
11   3f7245:      mov    0x8(%rsp),%rdx
12   3f724a:      sub    %fs:0x28,%rdx
13@@ -881655,7 +881655,7 @@ Disassembly of section .text:
14   4021e3:      cmp    $0x3,%eax
15   4021e6:      ja     402374 <LegacyScriptPubKeyMan::ImportScriptPubKeys(std::set<CScript, std::less<CScript>, std::allocator<CScript> > const&, bool, long)+0x264>
16   4021ec:      mov    %eax,%eax
17-  4021ee:      lea    0x2788eb(%rip),%rcx        # 67aae0 <CSWTCH.5100>
18+  4021ee:      lea    0x2788eb(%rip),%rcx        # 67aae0 <CSWTCH.5101>
19   4021f5:      mov    (%rcx,%rax,4),%eax
20   4021f8:      test   %eax,%eax
21   4021fa:      jne    4022bf <LegacyScriptPubKeyMan::ImportScriptPubKeys(std::set<CScript, std::less<CScript>, std::allocator<CScript> > const&, bool, long)+0x1af>
22@@ -924216,7 +924216,7 @@ Disassembly of section .text:
23   42e216:      mov    0x168(%rbp),%r12
24   42e21d:      lea    0x158(%rbp),%rax
25   42e224:      mov    %rax,(%rsp)
26-  42e228:      cmp    %r12,%rax
27+  42e228:      cmp    %rax,%r12
28   42e22b:      je     42e25a <CWallet::EncryptWallet(std::__cxx11::basic_string<char, std::char_traits<char>, secure_allocator<char> > const&)+0x4fa>
29   42e22d:      nopl   (%rax)
30   42e230:      mov    0x40(%r12),%rdi

in src/script/interpreter.cpp:1893 in fa09fe6e6d outdated

1889@@ -1890,7 +1890,7 @@ static bool VerifyWitnessProgram(const CScriptWitness& witness, int witversion,
1890             const valtype& script_bytes = SpanPopBack(stack);
1891             exec_script = CScript(script_bytes.begin(), script_bytes.end());
1892             uint256 hash_exec_script;
1893-            CSHA256().Write(&exec_script[0], exec_script.size()).Finalize(hash_exec_script.begin());
1894+            CSHA256().Write(exec_script.data(), exec_script.size()).Finalize(hash_exec_script.begin());

MarcoFalke commented at 7:07 am on May 1, 2021:

Fun fact: If CScript was a std::vector, this would be UB:

 0$ ./src/bitcoin-cli sendrawtransaction 020000000001015f784e93f6b843bd9ad1b34f86f0a7b034053ac5f18da5c8547aa6041ba24d3d0000000000ffffffff013905000000000000220020e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855010000000000
 1/usr/include/c++/11/debug/vector:438:
 2In function:
 3    std::__debug::vector<_Tp, _Allocator>::reference 
 4    std::__debug::vector<_Tp, 
 5    _Allocator>::operator[](std::__debug::vector<_Tp, 
 6    _Allocator>::size_type) [with _Tp = unsigned char; _Allocator = 
 7    std::allocator<unsigned char>; std::__debug::vector<_Tp, 
 8    _Allocator>::reference = unsigned char&; std::__debug::vector<_Tp, 
 9    _Allocator>::size_type = long unsigned int]
10
11Error: attempt to subscript container with out-of-bounds index 0, but 
12container only holds 0 elements.
13
14Objects involved in the operation:
15    sequence "this" @ 0x0x7fb68e7f8670 {
16      type = std::__debug::vector<unsigned char, std::allocator<unsigned char> >;
17    }
18error: Could not connect to the server 127.0.0.1:18443 (error code 1 - "EOF reached")
19
20Make sure the bitcoind server is running and that you are connecting to the correct RPC port.
21[1]+  Aborted                 (core dumped) ./src/bitcoind -noprinttoconsole

theStack commented at 8:20 am on May 1, 2021: member

Concept ACK

fanquake commented at 4:54 am on May 2, 2021: member

Concept ACK

Empact commented at 4:29 pm on May 2, 2021: member

How about splitting this up into a few more focused PRs? Test and bench changes are unrelated, no?

DrahtBot commented at 9:31 am on May 3, 2021: member

🕵️ @jamesob @achow101 @sipa have been requested to review this pull request as specified in the REVIEWERS file.

MarcoFalke commented at 9:54 am on May 3, 2021: member

How about splitting this up into a few more focused PRs? Test and bench changes are unrelated, no?

Thanks, done in #21840

MarcoFalke marked this as a draft on May 3, 2021

script: Replace address-of idiom with vector data() method fabb6dfe6e

MarcoFalke marked this as ready for review on May 4, 2021

refactor: Use CPubKey vector constructor where possible fa05dddc42

refactor: Use only one temporary buffer in CreateObfuscateKey face961109

refactor: Avoid &foo[0] on C-Style arrays

This is confusing at best when parts of a class use the
redundant operators and other parts do not.

faece47c47

refactor: Replace &foo[0] with foo.data() fac30eec42

MarcoFalke force-pushed on May 4, 2021

promag commented at 7:48 am on May 4, 2021: member

Concept ACK. Have you thought about some way to prevent new cases? Maybe a linter to detect for [0] with a list of exceptions, if necessary?

MarcoFalke commented at 8:00 am on May 4, 2021: member

A linter could be done in a follow-up (not by me, though)

practicalswift commented at 12:51 pm on May 4, 2021: contributor

Have you thought about some way to prevent new cases? Maybe a linter to detect for [0] with a list of exceptions, if necessary?

If anyone wants to write a linter as follow-up then this might be helpful as a starting point:

0$ git grep -E '&([a-zA-Z0-9_]+)\[0\]' -- "*.cpp" "*.h" ":(exclude)src/leveldb/" ":(exclude)src/secp256k1/" \
1       ":(exclude)src/tinyformat.h"

If the consensus opinion is that a linter is needed then I suggest writing it in Python and shelling out to git grep. @windsok’s nice file permission linter in #21740 is a good example of a linter written in Python which is using git grep to do the heavy lifting.

laanwj commented at 2:28 pm on May 4, 2021: member

Code review ACK fac30eec42c486ec1bfd696293040a7aa0f04625 This seems a clear non-controversial code improvement.

windsok commented at 3:23 pm on May 4, 2021: contributor

If the consensus opinion is that a linter is needed then I suggest writing it in Python and shelling out to git grep. @windsok’s nice file permission linter in #21740 is a good example of a linter written in Python which is using git grep to do the heavy lifting.

I’d be very happy to work on this if there is agreement there should be a lint test for it. If we want to create a new python based linter for it, it could also make sense to consolidate some of the existing cpp lint tests into it such as: https://github.com/bitcoin/bitcoin/blob/master/test/lint/lint-include-guards.sh https://github.com/bitcoin/bitcoin/blob/master/test/lint/lint-includes.sh https://github.com/bitcoin/bitcoin/blob/master/test/lint/lint-assertions.sh

practicalswift commented at 6:33 pm on May 4, 2021: contributor

cr ACK fac30eec42c486ec1bfd696293040a7aa0f04625: patch looks correct

promag commented at 8:32 am on May 5, 2021: member

Code review ACK fac30eec42c486ec1bfd696293040a7aa0f04625.

I’ve committed https://github.com/bitcoin/bitcoin/commit/fdf09779d3d48ab7c20d66b87f7527bf7cb2e6c5 that nukes more cases - adds prevector::const_iterator::data() which mimics std.

laanwj commented at 11:12 am on May 5, 2021: member

If we want to create a new python based linter for it, it could also make sense to consolidate some of the existing cpp lint tests into it such as:

Yes I like this line of thinking.

MarcoFalke merged this on May 5, 2021

MarcoFalke closed this on May 5, 2021

MarcoFalke deleted the branch on May 5, 2021

sidhujag referenced this in commit f333c3c391 on May 5, 2021

gwillen referenced this in commit 3850e85010 on Jun 1, 2022

DrahtBot locked this on Aug 16, 2022

refactor: Replace &foo[0] with foo.data() #21817

Conflicts