Consensus: Introduce static GetScriptFlags (mostly MOVEONLY) #10427

1. 
   jtimon commented at 2:36 am on May 19, 2017: contributor

   This have been proposed many times with different variations, including #9271 and ancestors, and proposals by @TheBlueMatt ( #10192 ) and @CodeShark (#6747) for example. I think this is something very easy to review and agree with. Regardless of later steps, let’s please take the first step already.
2. Consensus: Introduce static GetScriptFlags (mostly MOVEONLY) 0b3fcd42fc
3. fanquake added the label Refactoring on May 19, 2017
4. fanquake added the label Validation on May 19, 2017
5. 
   TheBlueMatt commented at 0:52 am on June 6, 2017: member

   I’m not sold on passing in the VersionBitsCache (which is the primary difference between this and the version at #10192) - why should ConnectBlock even be aware that there is a fancy cache making GetScriptFlags faster?
6. 
   jtimon commented at 12:43 pm on June 7, 2017: contributor

   why should ConnectBlock even be aware that there is a fancy cache making GetScriptFlags faster?

   Well, GetScriptFlags is aware of it anyway, so at a minimum, I think not calling IsWitnessEnabled from it is very reasonable.

   Regarding the question, ConnectBlock should is aware of the global versionbitscache because it uses it though GetScriptFlags, whether we chose to make that dependency explicit or not. But I guess this comes down to me liking globals being used explicitly instead of hidden when possible (and lower level functions that don’t need to use the globals and can use local parameters instead do that instead of using globals they don’t need to depend on). But from past discussions I think you don’t believe there’s anything wrong with using globals when you don’t need them or anything good with functions declaring all their dependencies explicitly as parameters, so I guess I can’t convince you on that.

7. 
   TheBlueMatt commented at 5:09 pm on June 7, 2017: member

   Guess its all a personal preference thing - make functions more static and pass globals in from higher up the stack, or put a bit more encapsulation in the lower-level functions and keep the globals only for those functions. I tend to prefer the second, myself, but open to other views.
8. 
   jtimon commented at 5:22 pm on June 7, 2017: contributor

   Thoughts on at least removing the use of IsWitnessEnabled() from your version of the change? That’s just removing the use of an unnecessary wrapper.
9. 
   TheBlueMatt commented at 5:52 pm on June 7, 2017: member

   I dont have any strong feelings, but IsWitnessEnabled is a much clearer expression of whats going on, and also keeps duplication of that check to one place, which is nice (especially given all the folks running around doing crazy 148/149/etc implementations).
10. 
    jtimon commented at 9:04 pm on June 7, 2017: contributor

    Whatever, I guess I can do this changes later if I ever get to the point where I can actually use them. Closing.
11. jtimon closed this on Jun 7, 2017

    ---

12. DrahtBot locked this on Sep 8, 2021

Contributors
jtimon TheBlueMatt

Labels
Refactoring Validation