refactor: Add FoundBlock.found member #22215

1. 
   ryanofsky commented at 9:46 pm on June 10, 2021: member

   This change lets IPC serialization code handle FoundBlock arguments more simply and efficiently. Without this change there was no way to determine from a FoundBlock object whether a block was found or not. So in order to correctly implement behavior of leaving FoundBlock output variables unmodified when a block was not found, IPC code would have to read preexisting output variable values from the local process, send them to the remote process, receive output values back from the remote process, and save them to output variables unconditionally. With FoundBlock.found method, the process is simpler. There’s no need to read or send preexisting local output variable values, just to read final output values from the remote process and set them conditionally if the block was found.

   ---

   This PR is part of the process separation project. The commit was first part of larger PR #10102.

2. Add FoundBlock.found member

   This change lets IPC serialization code handle FoundBlock arguments more
   simply and efficiently. Without this change there was no way to
   determine from a FoundBlock object whether a block was found or not. So
   in order to correctly implement behavior of leaving FoundBlock output
   variables unmodified when a block was not found, IPC code would have to
   read preexisting output variable values from the local process, send
   them to the remote process, receive output values back from the remote
   process, and save them to output variables unconditionally. With
   FoundBlock.found method, the process is simpler. There's no need to read
   or send preexisting local output variable values, just to read final
   output values from the remote process and set them conditionally if the
   block was found.

   5c5d0b6264
3. ryanofsky added this to the "In progress" column in a project

   ---

4. DrahtBot added the label interfaces on Jun 10, 2021
5. DrahtBot added the label Refactoring on Jun 10, 2021
6. theStack approved
7. 
   theStack commented at 8:08 pm on July 14, 2021: member

   Concept and code review ACK 5c5d0b62648e1b144b7b93c199f45265dac100e5
8. in src/node/interfaces.cpp:348 in 5c5d0b6264

   344@@ -345,6 +345,7 @@ bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLock<Rec
   345         REVERSE_LOCK(lock);
   346         if (!ReadBlockFromDisk(*block.m_data, index, Params().GetConsensus())) block.m_data->SetNull();
   347     }
   348+    block.found = true;
   

   ---

   ---

   

   fjahr commented at 11:07 pm on July 25, 2021:

   Should found be set to false in the first line (if (!index))?

   ---

   

   ryanofsky commented at 5:12 pm on July 28, 2021:

   Should found be set to false in the first line (if (!index))?

   The found member is just set to false in the constructor, and set to true when the block is found. I think setting it other places would just add complexity and possible confusion (and I guess be less efficient, not that efficiency matters).

   Implementation here is also consistent with the current FoundBlock design that only sets values when a block is found, and doesn’t set anything when a block isn’t found.

9. 
   fjahr commented at 11:07 pm on July 25, 2021: member

   Just to clarify that I am not missing any other aspects of this: (I haven’t dug into all the code where this will be used yet) if only fresh, empty FoundBlocks would ever be used we wouldn’t need this because we could check whether actual values were filled, so we could check any other value. If an actually already filled FoundBlock is passed, we will then need to ensure that the found variable is at least set to false before passing it in, otherwise, the value would still be true and make it seem like a block was found although no values were changed. found could also be flipped by the process failing to find the block. Instead of resetting the found member we could also use any other member to “flip” (like nullifying hash for example) and show that no block was found right? So this is mainly a readability optimization, correct?
10. 
    ryanofsky commented at 5:26 pm on July 28, 2021: member

    Thanks for review!

    Just to clarify that I am not missing any other aspects of this: (I haven’t dug into all the code where this will be used yet) if only fresh, empty FoundBlocks would ever be used we wouldn’t need this because we could check whether actual values were filled, so we could check any other value.

    To give more context, this field is only being used to help with IPC serialization of FoundBlock in/out parameters when forwarding method calls across processes in #10102. This line of internal IPC code is the only place that uses the new field, and normal wallet code just calling FoundBlock methods should never have a reason to use it or be concerned with it.

    The problem the new field solves for IPC code is just that that the IPC code has no way “to check whether actual values are filled” as you described. The FoundBlock class by design doesn’t designate any magic values and it avoids overwriting previous output values if a block doesn’t exist (https://github.com/bitcoin/bitcoin/pull/19425#discussion_r537917058). There are tradeoffs to this design, but I think the advantages are that it forces callers to initialize any values they are going to use, and it can also be convenient for scanning ranges or finding earliest or latest block attributes, because it keeps the current attribute value unchanged when a new block isn’t found, example https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.cpp#L2801-L2803

    If an actually already filled FoundBlock is passed, we will then need to ensure that the found variable is at least set to false before passing it in, otherwise, the value would still be true and make it seem like a block was found although no values were changed. found could also be flipped by the process failing to find the block. Instead of resetting the found member we could also use any other member to “flip” (like nullifying hash for example) and show that no block was found right? So this is mainly a readability optimization, correct?

    There’s really no case where an already used FoundBlock object is used to find another block. The class is basically only usable as temporary object. Checking for values to be flipped would be awkward for readability, but would also require making assumptions about outside implementation details (previous values passed by foundblock function callers, and new values set by foundblock function implementations) that there isn’t really an advantage to making.

11. 
    fjahr commented at 4:57 pm on August 1, 2021: member

    Thanks for addressing my comments @ryanofsky . I thought I might be missing something so thanks for confirming that current FoundBlock objects aren’t reused, I kind of interpreted that into the description of the PR. I would probably have explored adding a found() method to FoundBlock that checks for values being set but I agree with your comments on the assumptions that would be making.

    Code review ACK 5c5d0b62648e1b144b7b93c199f45265dac100e5

12. jamesob approved
13. 
    jamesob commented at 4:06 pm on August 10, 2021: member

    ACK 5c5d0b62648e1b144b7b93c199f45265dac100e5 (jamesob/ackr/22215.1.ryanofsky.refactor_add_foundblock)

    Simple, commonsense change - unambiguously indicate whether or not a block was found within chain interface return types.

    Small thing, but you could add some unittest coverage for this (even though the change is trivial).

    Built locally.

     0-----BEGIN PGP SIGNED MESSAGE-----
     1Hash: SHA512
     2
     3ACK 5c5d0b62648e1b144b7b93c199f45265dac100e5 ([`jamesob/ackr/22215.1.ryanofsky.refactor_add_foundblock`](https://github.com/jamesob/bitcoin/tree/ackr/22215.1.ryanofsky.refactor_add_foundblock))
     4
     5Simple, commonsense change - unambiguously indicate whether or not a block was found within chain interface return types.
     6
     7Small thing, but you could add some unittest coverage for this (even though the change is trivial).
     8
     9Built locally.
    10-----BEGIN PGP SIGNATURE-----
    11
    12iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmESo+cACgkQepNdrbLE
    13TwXX0g/7BXJdxQf+GUX/hElJAfkOvHqjCSukhlKgxtwXnMzKtV26LO2CnHcQ7JlL
    14jHmx4qgll1c6dvoxDzz0FXNNY8Sem4nOCdIARs9a6M5IRZaw/sbMNtLsQaqNdlOQ
    15BJ4Hv7vsYvdFfzSb6LnHUl+idPjOXYBeOwIbyzBoZ6aZfyLchobMIk9rMB6enuuT
    16/+E6EGYHQmTjRzhk5RoiYSNtARu0DCxez2v+Zi2L7FTcPpVMv4W22PrGVlnHsImP
    17YYX3b8htJ1dlbMUGg5FdmMuxyGGwVCR/yBWbNjODyhn/u1XdGj8JlfFiweUxCtK3
    183aVDHnwFW+MqRf2slo+EaJxDhYOowhQZDHe/sdygJ+50LTAwTwQ+V2n9JKODXhVK
    19NTTe84XIgF6F0nXIphy5bul+A9e+jIQiF5VxPGdAdGSWdihw5j1hUvTXKQmILOH7
    20PWTMiViYrM1k494H7rpGnZNE52rWLQZxmHM128K7Fy6JxcRo4V29QzLZzWJqd98e
    21OAR+fzVq9HkeFGD1yvbKfCyoaxvWCt28HlNIsythsAvopjiEjyBpUOCaJpaBvsJz
    22Keure159/+Y6XRHSERQ7QnjG9s09wbk3jK/ReGdMeMKE7i5i6dHOod3eVDn+0H99
    23l89iQgfwV5c5Yw1eESsxGmtD/Xfr9nR6/egztsoEOKfieoffEsg=
    24=pwZF
    25-----END PGP SIGNATURE-----
    

    0Tested on Linux-4.19.0-10-amd64-x86_64-with-glibc2.28
    1
    2Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis --enable-wallet --enable-debug --with-daemon --enable-natpmp-default CXX=/usr/lib/ccache/clang++ CC=/usr/lib/ccache/clang PYTHONPATH= --with-bignum=no --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --no-create --no-recursion
    3
    4Compiled with /usr/bin/ccache /usr/lib/ccache/clang++ -std=c++17 -mavx -mavx2 -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_srcdir)=.  -Wstack-protector -fstack-protector-all -fcf-protection=full -msse4 -msha -msse4.1 -msse4.2  i
    5
    6Compiler version: clang version 7.0.1-8+deb10u2 (tags/RELEASE_701/final)
    

14. Zero-1729 approved
15. 
    Zero-1729 commented at 1:51 pm on August 13, 2021: contributor

    crACK 5c5d0b6
16. 
    ryanofsky commented at 11:12 pm on August 17, 2021: member

    I think this PR is probably ready for merge. Two line change, multiple acks
17. fanquake merged this on Aug 18, 2021
18. fanquake closed this on Aug 18, 2021

    ---

19. sidhujag referenced this in commit cfb4bf97a5 on Aug 20, 2021
20. ryanofsky moved this from the "In progress" to the "Done" column in a project

    ---

21. DrahtBot locked this on Aug 18, 2022

Contributors
ryanofsky theStack fjahr jamesob Zero-1729

Labels
Refactoring interfaces