- User Since
- Nov 23 2018, 01:26 (60 w, 3 d)
@deadalnix returning for review due to slight changes. Note after our discussion that ScriptExecutionMetrics will remain a pure struct (as it is in this diff) since accumulation etc will happen elsewhere.
remove dependency on D4834
simplify changes to smallest possible (move-only on the for loop, and an extra update)
(For reference, this mechanism was introduced in D2047 which was basically a good fix.)
Sat, Jan 18
A note for curiosity: I tried to make the same kind of test using fundrawtransaction / signrawtransactionwithwallet RPC, however it took about 100 ms per tx. So with 500 txes, that made a 50 second long test, yuck! This test runs in 2 seconds.
simplify API ; rebase
rebase ; comments ; tweak to not use uint256
Fabien added some comments to this effect in D4642.
Going to drop this from my queue for now, but I still think it's worth considering in future.
The remaining inconsistencies are probably related to this fix:
OK, just a few failures remaining now, so it's better with that parent but something still wrong...
rebase onto D4929
Above failures on master, now will try rebased.
From what I can tell, this change is actually a bugfix and should have really been combined with D4802. Please get this landed soon.
Thu, Jan 16
Hmm weird that build failures happen with this patch, it smells like one of the prior diffs (which all got landed sort of at once) may actually be responsible. Safest option might be to revert all.
Tiny quibble but otherwise good.
For some reason my name appears as the Author for the commit, any idea why that happened?
Wed, Jan 15
There is another side-effect here, which is that bumping GetMinFee can technically bring sigops (and virtualsize) into play for wallet estimation. To accurately capture this would really suck, since we would need to communicate two fee rates (one for min relay fee in sat/byte, and one for the mempool rolling fee floor in sat/vbyte).
Tue, Jan 14
Seems OK I guess, we'll just have to remember to nuke test/tmp on our local builds occasionally as it fills up with test failures.
big overhaul to use ScriptExecutionMetrics
add another comment
Mon, Jan 13
Never mind, I was wrong -- Core does have these changes.
Out of curiosity, why are we backporting these when Core doesn't have them in their master?
Note: As far as closures go, CScriptCheck is slightly weird because it saves (some of) the results into the closure, rather than returning them.
I am thinking of making two changes here:
Or should it be called GetVirtualSizeIncrement instead of GetIncrementalVirtualSize ?
I have a feeling this needs some tests...
This could be integrated into the mentioned Diffs, but I figured the mathematical concepts here are a bit weird and deserve a separate think.
Ah fantastic, I was planning to do this at some point. Please fix rebase conflict (D4898 landed) but otherwise looks good, also include make bench-bitcoin (or whatever) in the test plan just to make sure that compiles too.
rebase onto D4906 for CI