Redundant code or latent bug #25682

1. 
   janus commented at 6:59 AM on July 23, 2022: none

   <!-- This issue tracker is only for technical issues related to Bitcoin Core. General bitcoin questions and/or support requests are best directed to the Bitcoin StackExchange at https://bitcoin.stackexchange.com. For reporting security issues, please read instructions at https://bitcoincore.org/en/contact/. If the node is "stuck" during sync or giving "block checksum mismatch" errors, please ensure your hardware is stable by running memtest and observe CPU temperature with a load-test tool such as linpack before creating an issue! -->

   <!-- Describe the issue -->

   Looking at the code below I can't understand how it is possible for the IF statement conditional expression to become true. cnt is already incremented before the IF statement. Is this not redundant? https://github.com/bitcoin/bitcoin/blob/master/src/script/script.h#L594

   I have opened a ticket, https://stackoverflow.com/questions/73088518/reduntant-code-possibly-in-bitcoin-script-h

   Expected behavior Possibly doesn't change the behavior.

   <!--- What behavior did you expect? -->

   Not Applicable Actual behavior

   <!--- What was the actual behavior (provide screenshots if the issue is GUI-related)? -->

   To reproduce

   <!--- How reliably can you reproduce the issue, what are the steps to do so? -->

   It is source code issue

   System information Not applicable

   <!-- What version of Bitcoin Core are you using, where did you get it (website, self-compiled, etc)? -->

   Main branch ..https://github.com/bitcoin/bitcoin

   <!-- What type of machine are you observing the error on (OS/CPU and disk type)? -->

   Not Applicable

   <!-- GUI-related issue? What is your operating system and its version? If Linux, what is your desktop environment and graphical shell? -->

   Not Applicable

   <!-- Any extra information that might be useful in the debugging process. -->

   <!--- This is normally the contents of a `debug.log` or `config.log` file. Raw text or a link to a pastebin type site are preferred. -->

2. janus added the label Bug on Jul 23, 2022
3. MarcoFalke removed the label Bug on Jul 23, 2022
4. MarcoFalke added the label Refactoring on Jul 23, 2022
5. MarcoFalke added this to the milestone 24.0 on Jul 23, 2022
6. 
   fjahr commented at 11:14 AM on July 24, 2022: contributor

   @janus you are correct, but possibly this is not the final version of this function, since this is fairly recently added code and I think the work on Miniscript is still in progress.

   CC: @darosior

7. 
   darosior commented at 9:21 AM on July 25, 2022: member

   Thanks for the ping Fabian.

   Janus, you are correct. My bad, i mistakenly moved it there while fixing a previous bug with cnt. The intention here is to mimic ret.empty(), so moving it after the condition makes this small optimization (moving instead of inserting) possible:

   diff --git a/src/script/script.h b/src/script/script.h
   index 3b799ad637..1e5f694d52 100644
   --- a/src/script/script.h
   +++ b/src/script/script.h
   @@ -588,7 +588,6 @@ CScript BuildScript(Ts&&... inputs)
        int cnt{0};
    
        ([&ret, &cnt] (Ts&& input) {
   -        cnt++;
            if constexpr (std::is_same_v<std::remove_cv_t<std::remove_reference_t<Ts>>, CScript>) {
                // If it is a CScript, extend ret with it. Move or copy the first element instead.
                if (cnt == 0) {
   @@ -600,6 +599,7 @@ CScript BuildScript(Ts&&... inputs)
                // Otherwise invoke CScript::operator<<.
                ret << input;
            }
   +        cnt++;
        } (std::forward<Ts>(inputs)), ...);
    
        return ret;
   

   I'll open a PR with this patch.

8. 
   janus commented at 9:41 AM on July 25, 2022: none

   Thanks for the ping Fabian.

   Janus, you are correct. My bad, i mistakenly moved it there while fixing a previous bug with cnt. The intention here is to mimic ret.empty(), so moving it after the condition makes this small optimization (moving instead of inserting) possible:

   diff --git a/src/script/script.h b/src/script/script.h
   index 3b799ad637..1e5f694d52 100644
   --- a/src/script/script.h
   +++ b/src/script/script.h
   @@ -588,7 +588,6 @@ CScript BuildScript(Ts&&... inputs)
        int cnt{0};
    
        ([&ret, &cnt] (Ts&& input) {
   -        cnt++;
            if constexpr (std::is_same_v<std::remove_cv_t<std::remove_reference_t<Ts>>, CScript>) {
                // If it is a CScript, extend ret with it. Move or copy the first element instead.
                if (cnt == 0) {
   @@ -600,6 +599,7 @@ CScript BuildScript(Ts&&... inputs)
                // Otherwise invoke CScript::operator<<.
                ret << input;
            }
   +        cnt++;
        } (std::forward<Ts>(inputs)), ...);
    
        return ret;
   

   I'll open a PR with this patch. @fjahr @darosior Thanks for your comments.

   I hope to start contributing patches in the future.

9. darosior cross-referenced this on Jul 26, 2022 from issue script: actually trigger the optimization in BuildScript by darosior
10. MarcoFalke closed this on Jul 30, 2022

    ---

11. sidhujag referenced this in commit b53c7fa0a7 on Aug 1, 2022
12. bitcoin locked this on Jul 30, 2023

Contributors

janusfjahrdarosior

Milestone
24.0