refactor: performance-for-range-copy in psbt.h #30253

1. 
   maflcko commented at 11:12 am on June 9, 2024: member

   A copy of the partial signatures is not required before serializing them.

   For reference, this was found by switching the codebase to std::span (not sure why it wasn’t found with Span though):

   0./psbt.h:240:23: error: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy,-warnings-as-errors]
   1  240 |             for (auto sig_pair : partial_sigs) {
   2      |                       ^
   3      |                  const  &
   

2. refactor: performance-for-range-copy in psbt.h fab01b5220
3. 
   DrahtBot commented at 11:12 am on June 9, 2024: 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	theStack, tdb3

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

4. DrahtBot added the label Refactoring on Jun 9, 2024
5. 
   maflcko commented at 11:13 am on June 9, 2024: member

   Reference CI run: https://github.com/bitcoin/bitcoin/runs/25847037350
6. in src/psbt.h:240 in fab01b5220

   236@@ -237,7 +237,7 @@ struct PSBTInput
   237 
   238         if (final_script_sig.empty() && final_script_witness.IsNull()) {
   239             // Write any partial signatures
   240-            for (auto sig_pair : partial_sigs) {
   241+            for (const auto& sig_pair : partial_sigs) {
   

   ---

   ---

   

   Gary19751957 commented at 1:16 pm on June 9, 2024:

   0            for (const auto& sig_pair : partial_sigs) {
   

7. Gary19751957 changes_requested
8. 
   tdb3 commented at 3:15 pm on June 9, 2024: contributor

   Approach ACK. Less copying makes sense. Briefly searched for other instances in psbt.h where potentially unnecessary copying is done.

   Would it also make sense to adjust these references to const (or is there a specific reason why const should not be used here)? Adjusted to const auto& entry and ran unit and functional tests (no issues observed).

   https://github.com/bitcoin/bitcoin/blob/fab01b5220c28a334b451ed9625bd3914c48e6af/src/psbt.h#L355-L359

   https://github.com/bitcoin/bitcoin/blob/fab01b5220c28a334b451ed9625bd3914c48e6af/src/psbt.h#L780-L784

   https://github.com/bitcoin/bitcoin/blob/fab01b5220c28a334b451ed9625bd3914c48e6af/src/psbt.h#L1013-L1017

9. 
   maflcko commented at 3:25 pm on June 9, 2024: member

   Less copying makes sense. Briefly searched for other instances in psbt.h where potentially unnecessary copying is done.

   The examples you provided are already const references. In C++, when calling a member method that is marked const, member access will be const. Moreover, auto& is a reference that hides the type, including the const-ness, so const auto& and auto& should be identical in this context (const references that do not copy), and no further change should be needed.

10. theStack approved
11. 
    theStack commented at 5:51 pm on June 9, 2024: contributor

    utACK fab01b5220c28a334b451ed9625bd3914c48e6af
12. DrahtBot requested review from tdb3 on Jun 9, 2024
13. 
    tdb3 commented at 5:54 pm on June 9, 2024: contributor

    no further change should be needed.

    Thanks. ACK fab01b5220c28a334b451ed9625bd3914c48e6af

14. fanquake merged this on Jun 10, 2024
15. fanquake closed this on Jun 10, 2024

    ---

16. maflcko deleted the branch on Jun 10, 2024

Contributors
maflcko DrahtBot Gary19751957 tdb3 theStack

Review Requested
tdb3

Labels
Refactoring