< bitcoin-git>
bitcoin/master 490da63 Kristaps Kaupe: Make lint-includes.sh work from any directory
< bitcoin-git>
bitcoin/master 761fe07 MarcoFalke: Merge #16768: test: Make lint-includes.sh work from any directory
< bitcoin-git>
[bitcoin] MarcoFalke merged pull request #16768: test: Make lint-includes.sh work from any directory (master...lint-includes-anydir) https://github.com/bitcoin/bitcoin/pull/16768
< midnightmagic>
ah. so the code isn't old. it's pre-warning in gcc saying we're doing it right now, lucky us. what a bizarre warning to emit.
< sipa>
if you're linking pre-gcc-7.1 and gcc-7.1 compiled code, things will break
< sipa>
the compiler can't know whether you're doing that or not
< sipa>
generally ABIs don't change like that
< midnightmagic>
Ah, an ABI thing. Okay less weird to me. Thank you.
< bitcoin-git>
bitcoin/master 2457aea Samuel Dobson: Assert that the HRP is lowercase in Bech32::Encode
< bitcoin-git>
bitcoin/master 5667b0d Wladimir J. van der Laan: Merge #16792: Assert that the HRP is lowercase in Bech32::Encode
< bitcoin-git>
[bitcoin] laanwj merged pull request #16792: Assert that the HRP is lowercase in Bech32::Encode (master...201909_bech32_hrp_check) https://github.com/bitcoin/bitcoin/pull/16792
< bitcoin-git>
[bitcoin] meshcollider opened pull request #16807: Let validateaddress locate error in Bech32 address (master...201909_bech32_error_detection) https://github.com/bitcoin/bitcoin/pull/16807
< aj>
jnewbery: if you like it, put a PR on it? :)
< aj>
jnewbery: i mean, feel free to merge it into your PR :)
< jnewbery>
aj: testing now
< aj>
jnewbery: thinking about it, could just set it to 0 for regtest (since no historical versionbit stuff for segwit etc needed there); which means it could be set straight after the buried heights not after -segwitheight is worked out
< stevenroose>
Is there already a C++ implementation of the taproot tagged hashes? I'm looking for some example values to test an implementation against.
< wumpus>
note that the feature freeze for 0.19 is in 10 days
< BlueMatt>
(and its a small diff!)
< wumpus>
so we likely want to prioritize features that are close to ready now
< wumpus>
right
< wumpus>
would be nice if that makes it in
< gleb>
#16702 is done code-wise (I think), but perhaps it's suboptimal to add it to high prio at this point when we're close to the feature freeze.
< gribble>
https://github.com/bitcoin/bitcoin/issues/16702 | p2p: supplying and using asmap to improve IP bucketing in addrman by naumenkogs · Pull Request #16702 · bitcoin/bitcoin · GitHub
< wumpus>
gleb: yes, we might want to keep that for 0.20
< wumpus>
also requires too much review to still make it to 0.19 anyway
< wumpus>
still, great to hear you're making progress!
< sipa>
yeah, that sounds like too close to make it
< jeremyrubin>
Not super critical, and it's relatively new so not much time for review, but would help me to get #16766 as my work on OP_SECURETHEBAG wallet support depends on it
< gribble>
https://github.com/bitcoin/bitcoin/issues/16713 | Ignore old versionbit activations to avoid unknown softforks warning by jnewbery · Pull Request #16713 · bitcoin/bitcoin · GitHub
< achow101>
16713 is the simpler, easier to review fix
< achow101>
But I think it would be better to eventually get rid of these deployment parameters and have a more permanent solution that lets us reuse bits in the future
< wumpus>
what disadvantages does it have compared to the other one?
< MarcoFalke>
bits can be reused as long as they don't overlap in time
< aj>
16713 is updated as of a few hours to be even simpler; i think it lets us reuse bits fine?
< wumpus>
e.g. why are there two open at all?
< wumpus>
MarcoFalke: yup
< MarcoFalke>
I already closed the one by achow101 *hides*
< aj>
MarcoFalke: smooth
< wumpus>
ok, that concludes the discussion then I guess :)
< achow101>
my understanding of how consensus.vDeployments worked was that you couldn't define two forks with the same bit since they'd have to occupy the same index position and that's not possible
< wumpus>
oh
< achow101>
but 16713 has changed since I last reviewed it and it looks very different
< wumpus>
sorry
< MarcoFalke>
oh, maybe you are right when it comes to the implementation. Though, BIP9 does allow it
< wumpus>
BIP9 definitely allows it, that was an important part of the design
< aj>
achow101: vDeployments just matches the enum, the actual bits used are independant, and just have to not overlap per their corresponding timestamps
< sipa>
i haven't looked at the code in a while, but it was certainly intended to permit reuse of bits
< sipa>
the deployments array index is independent from the bip9 bit position
< instagibbs>
yep
< aj>
achow101: see VersionBitsConditionChecker::Mask in versionbits.cpp, it pulls out the .bit field from BIP9Deployment struct
< achow101>
i'm probably wrong
< instagibbs>
It was confusing the first time I read it, I came to same wrong conclusion
< aj>
achow101: there's also a unit test for overlapping bit usage on mainnet in src/test/versionbits_tests.cpp, "Verify that the deployment windows of [...]"
< wumpus>
if even experienced developers get confused by it, some documentation/comments might help in that case
< wumpus>
#action please review #16713 so that it can be merged asap
< MarcoFalke>
I think it shouldn't matter in practice, hopefully there are less than 27 softforks in the future
< gribble>
https://github.com/bitcoin/bitcoin/issues/16713 | Ignore old versionbit activations to avoid unknown softforks warning by jnewbery · Pull Request #16713 · bitcoin/bitcoin · GitHub
< wumpus>
given that 'the future' is unbounded, that's a difficult statement
< wumpus>
#topic Rolling UTXO set hash (fjahr)
< fjahr>
Did anyone have time to look at the proposal? Any questions?
< achow101>
link?
< instagibbs>
fjahr, pitch it to us in a few sentences :)
< sipa>
fjahr: it's not clear to me exactly what you're proposal :)
< fjahr>
I have picked up Pieter Wuille's proposal from 2017 to use a rolling hash for the UTXO set hash. It deals with the problem of a long computation time of the UTXO set hash which results in a slow RPC call gettxoutsetinfo (can take several minutes depending on hardware). I investigated three hash functions: MuHash, ECMH and LtHash and started implementing them in Bitcoin Core for comparison. However only MuHash
< fjahr>
has a rolling hash implementation so far and my LtHash code is not as optimized as MuHash and ECMH. I am looking for feedback on concept, which choice to make for the hash function and implementation details before filing a PR to Bitcoin Core.
< sipa>
it's a complicated question, as the right design may depend on how we intend to use the construction
< wumpus>
fjahr: thanks for picking it up!
< MarcoFalke>
I think one use case is assumeutxo
< fjahr>
sipa: by construction you mean the hash, right?
< sipa>
fjahr: right
< sipa>
assumeutxo (at least with a from-network-sync approach) will probably need more than just a single hash, as you want to be able to verify chunks etc
< sipa>
fjahr: if i recall correctly, the biggest question i had was how to prioritize computation time vs use-from-cache time
< sipa>
ECMH is slower to compute, but very fast to use from cached values (e.g. if you have precomputed the "diff" ECMH hash a transaction has, applying to a global sum is super fast)
< sipa>
but MuHash is faster is overall computation time
< fjahr>
From my benchmarks ECMH was faster overall but I am not sure why MuHash did not perform as you expect in you Mail
< sipa>
where do you see a discrepancy? in the hash-to-group-element operation, or in the multity?
< sipa>
*multiply?
< sipa>
actually i think the discussion of what hash to pick is less important for this meeting
< sipa>
we should probably focus on ways to integrate
< fjahr>
ok, and also if there is enough interest for this
< aj>
it's super cool
< jeremyrubin>
Are both constructions one-way?
< jeremyrubin>
How does that interplay with reorgs
< fjahr>
In terms of integration I chose to implement this as an index, any feedback on this?
< jeremyrubin>
Maybe I should read more out of band of here...
< sipa>
fjahr: that really depends what for
< fjahr>
jeremyrubin: hashes can also be removed again so no problem with reorgs
< sipa>
i think as an index it's hard to use precomputation
< sipa>
which you really want if you want the rolling part
< fjahr>
sipa: what would you suggest in terms of integration?
< sipa>
that really depends on what we want to use it for
< aj>
sipa: could have it rolling, but in it's own slightly delayed thread like tx indexes, without worrying about precomputation, at least if you don't want to enforce it in consensus
< wumpus>
there's 20 minutes to go and still a topic left, let's move on?
< sipa>
fjahr: let's talk more after the meeting
< fjahr>
sipa: sure
< wumpus>
#topic avoid loading every wallet transaction into memory (achow101)
< wumpus>
thanks
< sipa>
btw, the secp ECMH code you have looks great
< achow101>
This is a wallet topic, and probably better for the wallet meeting, but that's next week..
< wumpus>
is there any hurry? :-)
< achow101>
I was thinking about ways to reduce the memory footprint of loaded wallet files
< wumpus>
concept ACK anyhow
< jonasschnelli>
jup. also ack
< sipa>
achow101: seems hard
< achow101>
I was wondering if anyone who was more familiar with the wallet tracking part of the wallet knew if this was attempted before or would majorly break something?
< wumpus>
it always seemed unnecessary to me to keep all transactions, even historical ones with all outputs spent, in memory
< jonasschnelli>
There is a PR where all wallettxns where kept in mem
< jonasschnelli>
+different DB formst
< wumpus>
but yes, it's such a part of the current wallet design, it's definitely not going to be easy
< jonasschnelli>
#5686 (very old)
< gribble>
https://github.com/bitcoin/bitcoin/issues/5686 | [Wallet] replace BDB with internal append only (logdb) backend by jonasschnelli · Pull Request #5686 · bitcoin/bitcoin · GitHub
< sipa>
achow101: you mean not _show_ them anymore
< sipa>
or just not load them in memory?
< achow101>
not load them into memory, so it would read from disk when the full tx is needed
< wumpus>
right
< achow101>
I plan on just keeping UTXOs and txids in memory
< jarthur>
MarcoFalke: glad to see the flake8 update finally made it in and you won't have to keep rebasing it. :)
< jeremyrubin>
achow101: you can save *some* memory by repacking the CWalletTx struct I bet :)
< wumpus>
if you repack it you might as well re-retrieve it from the database
< achow101>
I want to avoid touching the wallet database, although that may not be possible
< jeremyrubin>
I meant just the fact that there's a lot of fields in the struct that are char then uint64 or something
< jeremyrubin>
which adds a lot of interior padding
< wumpus>
suure
< jeremyrubin>
I did say *some*
< achow101>
just a bit
< achow101>
At this rate, I think I'll have rewritten the entire wallet by this time next year
< sipa>
if done incrementally, i think that'd be great :)
< wumpus>
if you manage to do that you're the first ever person to achieve it, people have been saying that since 2011 or so though
< sipa>
haha
< sipa>
in 2011 the wallet was still part of main.cpp :p
< wumpus>
but I'm happy to see the work picking up lately on the wallet
< sipa>
we've come a long way
< wumpus>
yes, we've come a long way, for a long time there was hardly any interest in it and some devs were even arguing about removing it because it was just too bad
< wumpus>
it's definitely not like that anymore
< bitcoin-git>
[bitcoin] martinus closed pull request #16801: faster & less memory for sync: bulk pool allocator for node based containers (master...2019-08-bulkpoolallocator) https://github.com/bitcoin/bitcoin/pull/16801
< bitcoin-git>
[bitcoin] martinus reopened pull request #16801: faster & less memory for sync: bulk pool allocator for node based containers (master...2019-08-bulkpoolallocator) https://github.com/bitcoin/bitcoin/pull/16801
< instagibbs>
for all the hate on the wallet, the only other solutions are things like Electrum personal server, and other more ad hoc solutions. It's a project worth iterating on :)
< instagibbs>
> jeremyrubin wonders how big the largest wallet is
< instagibbs>
I've seen a multi GB testnet wallet, but that likely doesn't count
< instagibbs>
if you end up using Core wallet for "industrial" wallet it indeed slows significantly. rhavar probably has some anecdotes
< sipa>
change IsMine() to return true; and watch your wallet.dat explode :p
< instagibbs>
I guess the slowdown is just iterating through all txns anyways, so meh :)
< phantomcircuit>
instagibbs, it does count, don't down play my entirely valid usecase
< * phantomcircuit>
runs away
< warren>
instagibbs: I've seen a very large Bitcoin exchange in Asia that had suffered for years with a single wallet.dat for all customer wallets. It got to the point where ordinary RPC queries against that wallet would take several minutes. As a workaround for faster queries I told them to setup many parallel servers with bitcoind -txindex and watch-only wallets so that they could parallelize their lookups. I also warned that they check for block
< warren>
height agreement before querying anything.
< bitcoin-git>
[bitcoin] ch4ot1c opened pull request #16812: doc: Fix whitespace errs in .md files, bitcoin.conf, and Info.plist.in (master...docs/lint-markdown) https://github.com/bitcoin/bitcoin/pull/16812
< jb55>
might be handy to have a very large test wallet for performance testing