< fanquake>
Looking forward to Bionic tomorrow. Hopefully we can start using it on WSL soon after..
< kallewoof>
How are READWRITE(REF(CFlatData(ob))) done these days? I got barfed at when rebasing something.
< * kallewoof>
does git log serialize.h to find clues
< sipa>
kallewoof: depends, what type is ob?
< kallewoof>
sipa: std::vector<uint256>
< kallewoof>
Wait, no..
< kallewoof>
Sorry. Yes. std::vector<uint256>
< sipa>
with compile-time known length?
< kallewoof>
No, it's a merkle proof skip list
< sipa>
then what do you need the flatdata for?
< * kallewoof>
headscratch
< sipa>
just READWRITE(the_vector) will work fine
< kallewoof>
Is CFlatData only used for fixed length stuff?
< sipa>
CFlatData was historically used to write things as raw bytes, as opposed to the object's serializer
< sipa>
it's been replaced with native support for arrays
< sipa>
so you can READWRITE(blah) if blah is 'unsigned char blah[4]' for example, and it will just write 4 bytes
< sipa>
and with Span
< sipa>
but uint256 has a serializer that just writes 32 bytes
< kallewoof>
Okay, so actually reading the comment above, it seems Mark wanted to avoid using the Satoshi-defined compact size format as it was less efficient than the internal varint.
< kallewoof>
Hence why he wrote the length of the vector and the vector as CFlatData
< sipa>
i see
< sipa>
seems overkill to me
< kallewoof>
Yes, seems unnecessary.
< sipa>
as the native varint's aren't used anywhere in the p2p protocol already
< kallewoof>
*nod*
< sipa>
the same issue came up around bip152
< kallewoof>
How much more efficient are the varints compared to the compactsize version anyway?
< sipa>
it depends on the distribution
< kallewoof>
Right. Of course it does.
< sipa>
for some distributions the compactsize ones are better
< kallewoof>
But for a skip list in a merkle proof, I kind of don't think you'll ever have more than 10.
< kallewoof>
Or very seldom anyway. I could be wrong though.
< sipa>
then both serializations will give you just 1 byte anyway
< kallewoof>
Yeah
< sipa>
compactsize uses 1 byte up to (iirc) 252
< sipa>
varint only up to 127
< sipa>
but after that compactsize immediately jumps to 3 bytes
< sipa>
while varint only uses 2 up to 16000 ish
< kallewoof>
Oh, they're that different? I thought they were tweaks of the same thing.
< ossifrage>
Does bitcoin core do some sort of io maintenance task that gobbles up a bunch of disk IO?
< kallewoof>
Either way, the size of the size of the skip lists is a fraction of the size of the skip lists themselves which are 32 bytes per entry, so I doubt this matters. With 253 entries there are 8096 bytes of data aside from the size. 3 or 2 bytes isn't really gonna make a difference.
< ossifrage>
QThread just started generating a bunch of disk io without any large corresponding newtwork IO
<@wumpus>
jimpo: I can view it now, couldn't open it yesterday
<@wumpus>
strange
<@wumpus>
and yeah I couldn't see it had only three replies because I couldn't see the thing at all
< kallewoof>
sipa: Mark pointed out compactsize is malleable (e.g. 00000001 = 01).
< kallewoof>
sipa: Also noted concern about a non-optimized storage format in consensus code, which I can sort of understand. Gonna think this through some more.
< kallewoof>
I think the key issue is, what size will the average skip list be, in a merkle proof. If it's "thousands" then using varint sounds sensible. If it's "tens" it sounds overkill.
< promag>
#13028 is pretty straightforward, would love some ack/nack there
< bitcoin-git>
bitcoin/master 09b30db 251: Asserts that the tx version number is a signed 32-bit integer.
< bitcoin-git>
bitcoin/master 6e67754 Wladimir J. van der Laan: Merge #12436: [rpc] Adds a functional test to validate the transaction version number in the RPC output...
< bitcoin-git>
[bitcoin] laanwj closed pull request #12436: [rpc] Adds a functional test to validate the transaction version number in the RPC output (master...issue/11561/test-negative-transaction-version-numbers) https://github.com/bitcoin/bitcoin/pull/12436
< sdaftuar>
anyway not sure if that is enough to mitigate the concerns? i'm also happy to adopt a different approach, i tried several designs last year, but i could use some direction to help figure out the best path forward (if this is something we want)
< bitcoin-git>
bitcoin/master fac0db0 MarcoFalke: wallet: Make fee settings non-static members
< bitcoin-git>
bitcoin/master 476cb35 Wladimir J. van der Laan: Merge #12909: wallet: Make fee settings to be non-static members...
< bitcoin-git>
[bitcoin] laanwj closed pull request #12909: wallet: Make fee settings to be non-static members (master...Mf1804-walletMembers) https://github.com/bitcoin/bitcoin/pull/12909
< jtimon>
aj: oh, right, thanks
< jtimon>
aj: done
< BlueMatt>
MarcoFalke: I'd like to nominate #12998 for backporting. It has 0 risk of breaking supported configurations and virtually 0 risk of breaking anything else but is useful for a downstream project
< gribble>
https://github.com/bitcoin/bitcoin/issues/12998 | Default to defining endian-conversion DECLs in compat w/o config by TheBlueMatt · Pull Request #12998 · bitcoin/bitcoin · GitHub
< BlueMatt>
jonasschnelli: yea, I'll look into it when someone shows up with a 2fa
< promag>
AccessByTxid requires cs_main?
< jnewbery>
promag: I don't think shared pointers are required for wallet load/create. I'd rather not rebase on your PR unless necessary. I think loadwallet and g_wallet_manager can move forward in parallel
< bitcoin-git>
[bitcoin] jimpo closed pull request #12647: wallet: Fix possible memory leak in CreateWalletFromFile. (master...wallet-pointer) https://github.com/bitcoin/bitcoin/pull/12647
< sdaftuar>
sipa: actually, never mind about the segwit-hash-caching. now that we cache script validation success, we can achieve nearly all the benefit with a lot less work (and less controversial design) by just only doing the computation for transactions that aren't in the script cache