< bitcoin-git>
[bitcoin] achow101 opened pull request #10788: Replace ismine with producesignature check in witnessifier (master...fix-addwitnessaddress) https://github.com/bitcoin/bitcoin/pull/10788
< bitcoin-git>
[bitcoin] stevendlander opened pull request #10789: Punctuation/grammer fixes in rpcwallet.cpp (master...cli-punctuation-standardization) https://github.com/bitcoin/bitcoin/pull/10789
< jtimon>
mhm, can't I use something I have in /etc/host with -rpcallowip ? only hardcoded ips ?
< jtimon>
not even localhost it seems :(
< sipa>
jtimon: it has to be a netmask, i think
< sipa>
so no
< jtimon>
found getent hosts myhostname | awk '{ print $1 }', hope it solves my issue
< jtimon>
thanks for confirming
< bitcoin-git>
[bitcoin] MeshCollider opened pull request #10790: Use vector::data() instead of &vch[0] in base58.cpp (master...prefer-vector-data) https://github.com/bitcoin/bitcoin/pull/10790
< bitcoin-git>
bitcoin/master ff6a834 Matt Corallo: Use TestingSetup to DRY qt rpcnestedtests
< bitcoin-git>
bitcoin/master 3a19fed Matt Corallo: Make ValidationInterface signals-type-agnostic...
< bitcoin-git>
bitcoin/master cda1429 Matt Corallo: Give CMainSignals a reference to the global scheduler...
< bitcoin-git>
[bitcoin] laanwj closed pull request #10179: Give CValidationInterface Support for calling notifications on the CScheduler Thread (master...2017-01-wallet-cache-inmempool-3) https://github.com/bitcoin/bitcoin/pull/10179
< bitcoin-git>
bitcoin/master cfaef69 Alex Morcos: remove default argument from GetMinimumFee
< bitcoin-git>
bitcoin/master d507c30 Alex Morcos: Introduce a fee estimate mode....
< bitcoin-git>
bitcoin/master e0738e3 Alex Morcos: remove default argument from estimateSmartFee
< bitcoin-git>
[bitcoin] laanwj closed pull request #10589: More economical fee estimates for RBF and RPC options to control (master...rpcestimatechoice) https://github.com/bitcoin/bitcoin/pull/10589
< morcos>
I think #10712 can also be merged. sipa: you left just a nit, were you still reviewing? I'll skip the nit since the current commit has 3 ack's
< morcos>
BlueMatt: How does it know what index to create key at if the keypool is empty? (same question before your PR)
< BlueMatt>
hmm?
< BlueMatt>
it doesnt matter if the pool is empty, use index 0?
< BlueMatt>
the indexes dont really have a purpose, its more of a set
< wumpus>
looking at 10235
< morcos>
yeah i just figured that out, that the indices are only for keypool management
< BlueMatt>
yea
< morcos>
but are you sure there is not a bug now that you have two keypools
< BlueMatt>
well afterwards you should still make sure they keyids are unique, ie creating at the max of both sets
< morcos>
if external runs out, couldn't you create a new key in the external pool with the same index as one in the internal pool
< BlueMatt>
i hope that pr doesnt do that
< morcos>
i'm not sure if there is a bug, i'm making my way through all this logic for the first time, but i think it would be a lot clearer if you figured out nInternalEnd and nExternalEnd and maxed them for nEnd
< morcos>
actually it seems even worse than that
< morcos>
i just don't think this logic works with two pools
< BlueMatt>
hmm?
< morcos>
imagine internal = {1,3} and external = {2,4}
< morcos>
then external runs out
< BlueMatt>
you can reuse indexes
< morcos>
you'll now generate 4 as the next internal key or external key
< BlueMatt>
thats no problem
< morcos>
oh yeah, i guess thats ok
< morcos>
ok, so ignore me
< morcos>
maybe it's pretty clear.
< morcos>
what about the pool on disk though? it seems like you shouldn't start reusining indices until you're sure they aren't in there?
< BlueMatt>
hmmmmmm, yea, there is a bug there, but I didnt introduce it
< morcos>
yeah, but made it much more likely to get hit i think
< BlueMatt>
dont think so?
< BlueMatt>
ReserveKeyFromKeyPool, (keypool now empty), TopUpKeyPool, fucked
< BlueMatt>
I dont think i made a real difference to the likelyhood there
< morcos>
well before you start over again at 1, so it's unlikely the initial indices are still in disk pool
< morcos>
but with yours you could easily reuse a recent one as per my {1,3} {2,4} example above
< BlueMatt>
depends on your keypool size
< BlueMatt>
true
< morcos>
sure, seems like a bug either way
< BlueMatt>
anyway, its only really likely if you have a small pool, but still not good
< BlueMatt>
unless you object I'll fix it in a separate pr?
< wumpus>
what is the worst that can happen if an index is inadvertently reused?
< morcos>
so what happens, do you overwrite the old keypool entry with the new one on disk? and seems like you could either return the reserved key or use it, and either case might cause problems
< BlueMatt>
jonasschnelli: where are you on #10650? We
< BlueMatt>
re dangerously close to having multiwallet slip to 16
< BlueMatt>
wumpus: yea, not clear to me either, intuitively it should be rather minimal, not like we're clearing private keys or anything, but I could see it hitting an assert somewhere
< morcos>
BlueMatt: I suppose I have a slight preference for you adding another commit to that PR to fix it.
< wumpus>
is everything necessary for multiwallet tagged as 0.15?
< BlueMatt>
i think its just 10650 at this point, no?
< wumpus>
yes, I think so, and that one is tagged, good
< BlueMatt>
morcos: heh, ok, I was trying to avoid polluting the bugfix-vs-not distinction, but I suppose the performance regression is somewhat bugfix-y anyway
< morcos>
64-bits is a big number.. seems pretty safe to just have an in-memory nextIndex that is initialized by taking max of the on-disk last entry in the set?
< morcos>
sets
< wumpus>
if you do that, please do make it per-wallet, not global
< BlueMatt>
yes, that was my preferred fix
< wumpus>
but if it's 64-bit I don't think we have to be worried about reusing
< wumpus>
unless small numbers are preferable because they are more efficient for some reason
< wumpus>
people have to be able to maintain that code, also people that are not you
< BlueMatt>
i didnt write it
< BlueMatt>
I'm fixing...
< wumpus>
that function in fibre looks more understandable
< wumpus>
it doesn't do anything really crazy and it does it step by step
< BlueMatt>
ok, fixed on 10235
< BlueMatt>
lol, clearly I need to try harder to write garbage iterator code in fibre then
< wumpus>
well as long as you don't inadvertently add UB
< wumpus>
but if you really want to, make sure it's all in one expression and the order of operations, as well as what applies to what isn't clear
< wumpus>
yes, this is much better, thanks
< BlueMatt>
heh, I was joking, that stuff is hard enough to maintain when I go back once every three months and rewrite big chunks to get another 2 milliseconds out of it :p
< BlueMatt>
without remembering the threading model that will cause a segfault-in-a-million if you break it
< wumpus>
I can imagine
< cfields>
BlueMatt: fwiw, you can use *.rend() if !empty()
< cfields>
er, rbegin
< BlueMatt>
yea, that too
< BlueMatt>
wumpus: err, nvm, easier to just fix the first bug than bother with simplifying that code
< wumpus>
I'm afraid #10712 doesn't build on top of current master
< BlueMatt>
well, I'll lave 10235 with the fix for now, since it should be an easy merge
< BlueMatt>
and then it'll get removed in the next pr, which will need a whole review cycle
< morcos>
yeah conflict with my other PR you just merged but in a newly added line, will fix 10712
< bitcoin-git>
[bitcoin] TheBlueMatt opened pull request #10795: No longer ever reuse keypool indexes (master...2017-07-wallet-keypool-overwrite) https://github.com/bitcoin/bitcoin/pull/10795
< sipa>
these are some preliminary benchmarks for a reindex-chainstate up to height 450000 (before assumevalid, so no signature checking), with infinity dbcache, for various sha256 implementations:
< bitcoin-git>
bitcoin/master 253cd7e Alex Morcos: Only reserve key for scriptChange once in CreateTransaction...
< bitcoin-git>
bitcoin/master 0f402b9 Alex Morcos: Fix rare edge case of paying too many fees when transaction has no change....
< bitcoin-git>
bitcoin/master e8b9523 Wladimir J. van der Laan: Merge #10712: Add change output if necessary to reduce excess fee...
< sipa>
rorx and sse are from wumpus' work to include some asm optimized versions
< bitcoin-git>
[bitcoin] laanwj closed pull request #10712: Add change output if necessary to reduce excess fee (master...addchangehwenoverpaying) https://github.com/bitcoin/bitcoin/pull/10712
< sipa>
the shani version is using code from cryptopp
< BlueMatt>
lol, Make Bitcoin CryptoPP Again
< BlueMatt>
thats a pretty impressive gain
< sipa>
unfortunately, very few systems right now have sha-ni instructions
< BlueMatt>
yes, but that should change over time
< BlueMatt>
the sha-ni in intel and amd are identical, no?
< sipa>
afaik, there is exists no "sha-ni in intel" yet :)
< wumpus>
that's always the dilemma with new instructions, the systems that have that instruction probably least need the optimization :)
< sipa>
sse is present in nearly every x86_64 cpu, i think
< wumpus>
yes, sse is present in all of them
< wumpus>
even sse2 should be in all of them
< BlueMatt>
sipa: hmm? I was under the impression they had announced something like the next arch will get it
< gmaxwell>
sse2 is required by x86_64.
< BlueMatt>
no shipping ones, obviously
< wumpus>
those are not exactly new instructions though :) anyhow supporting shani as well is nice, just takes time and it will be in near everything...
< gmaxwell>
here is something funny: compilng bitcoin with -march=native makes the sha2 in C most of the speed of the sse one. (compiler manages to use rorx)
< gmaxwell>
Also the x86 simd support will make it easier to plug in arm versions; which will be more important for speed.
< BlueMatt>
wow, yay compiler smarts
< gmaxwell>
sipa: there is an intel chip with sha-ni, but it's some crappy atom processor.
< BlueMatt>
<wumpus> that's always the dilemma with new instructions, the systems that have that instruction probably least need the optimization :)
< BlueMatt>
heh, guess not, then
< sipa>
gmaxwell: compiling all of bitcoind with -march=native actually makes it overall another 100s faster to sync
< gmaxwell>
well aformentioned atom is some server atom that isn't very widely deployed from what I can tell.
< BlueMatt>
do you have lto benchmarks?
< sipa>
not yet, but i can create
< wumpus>
yes, march=native is nice, everyone compiling bitcoind locally should use it
< gmaxwell>
#9804 looks like a good change that is getting forgotten. (I say that as someone whos only concept acked so far...)
< gribble>
https://github.com/bitcoin/bitcoin/issues/9804 | Fixes subscript 0 ([0]) where should use (var.data()) instead. by JeremyRubin · Pull Request #9804 · bitcoin/bitcoin · GitHub
< wumpus>
with so many open PRs it's unavoidable that some things get forgotten, unfortunately
< wumpus>
but no, that one isn't
< wumpus>
I just referred people to review it today (because they opened a PR that was a strict subset of it)
< sipa>
wumpus: thanks for that, i had almost forgotten about jeremy's version
< BlueMatt>
lolwut
< BlueMatt>
ehh, wrong window
< sipa>
seems totally appropriate here
< BlueMatt>
ok, then I'll pretend I meant to
< sipa>
running benchmarks for lto and shani+lto as well now
< sipa>
will have numbers in a day...
< sipa>
interestingly, the shani+lto does not even contain any non-shani sha code, even though it's dispatched to a modifiable pointer
< BlueMatt>
heh
< BlueMatt>
oh nice
< sipa>
the compiler must notice there are no assignments to that pointer, and treats it as a constant
< BlueMatt>
yea, lto can be clever sometimes
< gmaxwell>
if compiler versions are still keeping up from ltoing by default perhaps we could add a --gofast configure flag to do -march=native -O3 -lto
< BlueMatt>
is O3 a win in bitcoin?
< sipa>
i guess i'll benchmark that too :)
< BlueMatt>
heh
< wumpus>
I don't see why something as basic as that would need a configure fla
< wumpus>
people that want to supply their own CXXFLAGS can do so already
< wumpus>
could just mention it in the build document
< wumpus>
a dedicated configure flag only makes sense if we use something for the gitian build, so people can do their own build with the same settings
< gmaxwell>
wumpus: fair enough
< wumpus>
but using -march=native would be a bad idea there, -flto might be a good one
< BlueMatt>
yea, flto could use its own configure flat...doing it manually is like 6 variables long
< gmaxwell>
well I think we should lto by default, but there was still some question of compiler support. (which I think was mooted by C++11, but perhaps I'm wrong, since we still haven't done it.)
< wumpus>
I'm not convinced anything but -O2 optimization should be default
< gmaxwell>
wumpus: why shouldn't it be lto-ing by default?
< wumpus>
cfields might have more of an opinion, but in my experience buildling open source software for lots of platforms, I've had lots of annoyances with software that tried to forcibly injects certain sets of optimizations flags
< wumpus>
it should be left up to the distribution ideally to set optimization flags
< BlueMatt>
gmaxwell: lto does not work on moderately-recent compilers
< wumpus>
well even if it did
< BlueMatt>
eg debian jessie
< sipa>
works fine here, and for release builds we control the environment entirely anyway
< BlueMatt>
well, i guess 4.9 isnt moderately-recent
< wumpus>
yes, for release builds flto would be good
< BlueMatt>
anyway, we support it, and lto doesnt work on it
< cfields>
wumpus: agreed. "-02 -g" is expected to be the default, I get really irritated when that's not the case
< wumpus>
cfields: exactly
< sipa>
benchmarking reindex with "-O2", "-O3", "-flto -O2", and "-flto -O3"
< BlueMatt>
nice
< wumpus>
don't get me wrong, flto would be great for gitian builds, and we could have a configure option for that, but adding non-standard optimization flags by default on configure tends to be really annoying in edge cases
< sipa>
cfields was concerned about having release builds deviate too much from developers-tested builds
< cfields>
I get the impression i should go ahead and wire up --enable-lto before someone sneaks in something unexpected :)
< gmaxwell>
BlueMatt: I do not believe that LTO fails to work on anything that can compile Bitcoin Core. It's been supported since GCC 4.7
< wumpus>
if you want to encourage people to build with optimization flags then just document it in e.g. build-unix.md
< wumpus>
don't do anything sneaky
< gmaxwell>
Our cflags are far from -O2 -g, though, we add about two dozen warning flags.
< wumpus>
warning flags don't do aything funny to the code
< cfields>
sipa: i think an --enable-lto alleviates that somewhat, since it makes it easy for devs to get the same results
< BlueMatt>
gmaxwell: I dunno, I've been building on lto for a long time and every debian jessie server I've ever installed has always failed to build lto
< sipa>
BlueMatt: due to some boost interaction, right?
< BlueMatt>
its an easy fix - just recompile util.cpp without lto and then it works, but its broken
< BlueMatt>
sipa: its not boost-specific, but thats where it fails
< BlueMatt>
it appears to be some general issue with destructors defined in headers
< cfields>
sipa: iirc i hit a boost::filesystem problem last i tried
< gmaxwell>
BlueMatt: good datapoint then.
< BlueMatt>
I've seen it fail on other things, but normally the thing you see is ~boost::filesystem()
< cfields>
but i assumed it was local
< BlueMatt>
ehh, sorry, not filesystem
< BlueMatt>
some boost::filesystem path ~ thing
< cfields>
sipa: ooh i know, we can wire it up with depends builds
< BlueMatt>
cfields: didnt you tell me you were gonna wire up a "trusting-trust" gcc build in depends for 15? :P
< BlueMatt>
I still dont see a PR
< cfields>
that way anyone using depends gets lto by default, and we also know what dep versions we're dealing with
< wumpus>
our good friend ubuntu trusty has gcc 4.8.4, hopefully that can do flto without probems (especially worried about windows! but we'll see)
< cfields>
BlueMatt: i don't think it's going to make it :(
< BlueMatt>
wumpus: I'd be kinda surprised if it did
< BlueMatt>
cfields: what? by this coming sunday? yea, somehow i doubt it
< wumpus>
can someone please just try instead of guess :-)
< BlueMatt>
+1
< BlueMatt>
go cfields, go
< wumpus>
I can do it, np, but thought cfields was going to
< cfields>
add lto to configure (and see if it works) ?
< wumpus>
cfields: yes, doing a gitian build with lto (for all platforms)
< BlueMatt>
oh, i thought we were talking about a "trusting trust" copy of gcc
< wumpus>
then seeing where/if the result crashes
< cfields>
sure, i can do that
< BlueMatt>
yea, ok, adding lto is much easier, that could make it
< wumpus>
BlueMatt: I thoughtwe were talking about optimizations
< * wumpus>
confused
< cfields>
adding lto to configure now, then seeing where it works.
< wumpus>
cool
< cluelessperson>
Hey guys, I'm looking at buying some new server hardware.
< cluelessperson>
is currently on my plate, but I was wondering if you might suggest something else that might be more beneficial towards bitcoin development, testing/QA?
< wumpus>
this really isn't the channel for server hw suggestions
< midnightmagic>
I sent him here.
< BlueMatt>
cluelessperson: anything with fast disk and lots of memory
< BlueMatt>
otherwise, O/T
< cluelessperson>
BlueMatt: I could do PCI-SSD, but what type of ram requirements are we talking?
< midnightmagic>
-- since he was hoping to help in some capacity with the development itself, and I know occasionally guidance in terms of e.g. differing hardware or stuff that would be more helpful than generic hardware is sometimes in demand.
< BlueMatt>
ok, no big deal, lets move to #bitcoin
< bitcoin-git>
[bitcoin] jonasschnelli closed pull request #8369: [FOR LATER USE][WIP][Wallet] add support for a flexible "set of features" (master...2016/07/wallet_features) https://github.com/bitcoin/bitcoin/pull/8369
< morcos>
ryanofsky: #10244 is marked high-priority, but seems to me from reading the conversation that although it is now concept acked it's targeted for post-0.15?
< gribble>
https://github.com/bitcoin/bitcoin/issues/10244 | [qt] Add abstraction layer for accessing node and wallet functionality from gui by ryanofsky · Pull Request #10244 · bitcoin/bitcoin · GitHub
< morcos>
just want to direct my review appropriately with 0.15 coming up
< jonasschnelli>
morcos: Yes. Agree. 10244 should probably removed until there is conceptual consensus (not cleat to me if we have that or not, which probably means we have not :) )
< ryanofsky>
i thought i had two concept acks
< jonasschnelli>
Oh. Yes. Right..
< ryanofsky>
but it's not intended for 15. maybe search for milestone:0.15.0
< jonasschnelli>
I take back my last comment then...
< jonasschnelli>
I think for 10244, it makes sense to rebase it after 0.15 has been split off and concentrate review (and hope for a merge)
< jonasschnelli>
It's a large PR though
< jonasschnelli>
+2,262 −1,152
< ryanofsky>
it's pretty much a mechanical change
< wumpus>
agreed
< wumpus>
it's not an end-user visible change, we should prioritize those for 0.15
< wumpus>
all the code cleanups pure refactorings can wait for 0.16
< morcos>
ok, i was viewing high priority as a subset of milestone:0.15, but perhaps that isn't correct
< jonasschnelli>
wumpus, ryanofsky: Argee about it beeing mechanical, though it still require line by line review by at least 2-3 guys.
< wumpus>
well maybe it is now, but in general it's just something that people can request review if it blocks future PRs from themselves
< wumpus>
but I agree ahving something high-prio that won't make 0.15 anyway is a bad idea
< wumpus>
right now
< jonasschnelli>
Yes. I had the same feeling. High-Prio in context of holding back the individual developer / focus of the individual developer.
< jonasschnelli>
About sipa's ser. changes (#10785), is it worth to benchmark the differences or do we expect non to little impact on performance?
< jonasschnelli>
(maybe its better to discuss some thing here instead of have to much discussion on the PR before we actually get reviews)
< jonasschnelli>
things*
< sipa>
jonasschnelli: i expect 0 impact on performance
< jonasschnelli>
sipa: Okay. Then it's probably not worth...
< ryanofsky>
it's been months since i've looked at it but, i remember duplicated logic in the aux request class & normal network download code
< jonasschnelli>
ryanofsky: I can't see any real duplication, maybe some lines that take care of passing to the instance and for the callback approach...
< ryanofsky>
i'm looking up my old comments
< jonasschnelli>
Great. Thanks..
< sipa>
jonasschnelli: it could be considered a bugfix though (but almost certainly not one that matters)
< jonasschnelli>
sipa: you mean the "cast ways the const"?
< jonasschnelli>
"cast away"
< ryanofsky>
jonasschnelli, my broader point is that i would like you to get some concept ack on your networking design from some other developers who know more about the networking code than me, before i spend more time reviewing it
< jonasschnelli>
ryanofsky: Yes. Sure. That makes sense.
< jonasschnelli>
About your comment, I saw that and I think its then not about duplication, it's a slightly different approach (which seems also to make sense).
< jonasschnelli>
I just like the have-its-own file approach for the reasons I wrote.. but lets get other opinions
< ryanofsky>
fillInNextBlocks is duplicative of FindNextBlocksToDownload, processWithPossibleBlock is duplicative of ProcessNewBlock was my point
< ryanofsky>
i don't think you are going to convince me that having two different control flows for regular downloads vs priority downloads is the way to go, but you don't have to convince me. i'm happy to accept that if others think it is a good idea
< jonasschnelli>
ryanofsky: I don't think its duplicative, if we would eliminate the class, that logic has just to move into net_processing.cpp/validation.cpp
< gmaxwell>
ryanofsky: I think others do not think it's a good idea, and already raised the concern.
< ryanofsky>
iirc, more logic could be shared if they were in the same place
< gmaxwell>
When that code was first implemented; ... made sense to do for a demo or whatnot.
< sipa>
just have a means to tell the network layer "i need block X, and here is callback for when you have it"
< jonasschnelli>
Okay. But the network layer hasn't to be stuffed into a single file/class?
< ryanofsky>
sipa, yes that is what i requested. i think the auxilliary class is too low level
< jonasschnelli>
But if the general opinion goes direction of having the logic in net_processing.cpp directly, then I'm fine with moving it there...
< sipa>
jonasschnelli: net_processing is already a single file
< ryanofsky>
jonasschnelli, i think agree networking code is monolithic. but that you have to find the right way to break it up and this does not seem like the right way
< jonasschnelli>
sipa: Yes. And I think having specialized functions (like the light client block rquests) in dedicated files makes sense.. rebasing, patches, code-overview
< sipa>
jonasschnelli: i really don't think block fetching code should be in more than 1 place
< gmaxwell>
we have a hard enough time keeping the two existing block requesting state machine functioning and maintained... really pretty ugly to add another seperate one for the wallet in another place.
< sipa>
it's arguably already too spread out in net_processing (dealing with invs, direct fetching, headers fetching, async fetching ...)
< jonasschnelli>
sipa: it's not fetching,.. is an object that hold information about what it should fetch and if it's done or not and eventuall the process flow
< sipa>
jonasschnelli: yes, that's probably how all the fetching code should work
< sipa>
keep information on what blocks are to be downloaded
< jonasschnelli>
ryanofsky gmaxwell: I think the out-of-band (auxiliary) requests need state, and therefore a class makes sense (when was the request initiated, how much is done, etc.) would you add that class into net_processing rathern then creating a new file?
< ryanofsky>
i think if you wanted to move code *out* of net_processing into your new download implementation, that would be nice. but just adding the new download implementation alongside does not seem seem so nice
< gmaxwell>
jonasschnelli: all block requests need state.
< gmaxwell>
jonasschnelli: we have to track what we've asked for, what we need, whats been recieved... and so on.
< jonasschnelli>
gmaxwell: yes. But here it is a user-requested range
< sipa>
jonasschnelli: in the current case, the user is the block validation logic
< sipa>
you just want to add another user
< gmaxwell>
^
< jonasschnelli>
sipa: can't follow the "user is the block validation logic" :/
< ryanofsky>
to make discussion more concrete, what do you think of my blocksToDownloadFirst suggestion?
< jonasschnelli>
ryanofsky: Yes. Make sense. But still, the requester (assume wallet) will need to blocks in a clear sequence (assume A, B, C, D and not C, D, A, B).
< jonasschnelli>
Therefore there must be something that takes care of the state of a wallet/user initiated out-of-band block request and make sure, it will be passed in in sequence
< jonasschnelli>
IMO its about "requesting n amount of "range of blocks" rather then just requesting blocks
< gmaxwell>
jonasschnelli: right now the consensus logic requests the block download machinery to download blocks, keep track of the requests and what not.
< ryanofsky>
i'm not sure i see the problem. that state could be tracked inside the wallet. wallet just needs to request blocks, get notifications when they come in, then request more blocks
< gmaxwell>
jonasschnelli: sipa is pointing out that what you're doing is just adding an addition set of callers.
< gmaxwell>
jonasschnelli: why does the wallet need blocks in order?
< jonasschnelli>
gmaxwell: if we assume the wallet does show to the user what it had processed it would probably look strange if the wallet would update the balance, tx-list of block that come in out of order
< jonasschnelli>
Also, detecting re-orgs would be difficult?
< ryanofsky>
can't wallet keep track of it's own requests and ignore anything it wants to ignore?
< gmaxwell>
if the wallet is just saying how far it had processed, couldn't it just show the minimum height processed so far? showing things out of order would certantly allow things faster.
< jonasschnelli>
ryanofsky: Yes. If we assume only wallets want that. What about the other user cases, ZMQ, light-client "middleware" (Oh I had this term)
< ryanofsky>
jonasschnelli, that's why i was asking for some kind of design doc, going into the what specifically you envision there
< jonasschnelli>
gmaxwell: hmm.. I guess the user would look at the transaction list and see a possible incoming transaction while a older expected incoming transaction is missing, regardless of how the wallet communicates "minimum process height"
< sipa>
jonasschnelli: right now, the only thing that "needs" blocks is the block validation
< sipa>
you're adding something else that needs blocks
< jonasschnelli>
ryanofsky: Not only, #10794 is purely RPC/ZMQ
< sipa>
both of them should be seen as users of the block fetching logic
< jonasschnelli>
sipa: Ah. I understand now
< jonasschnelli>
sipa, ryanofsky: The idea in 10794 is to have the validation triggered download logic in tact (minimal impact) while preferring out-of-band requests. And I tried to keep the logic, state-machine outside of net_processing.cpp to allow later code splits
< sipa>
i think that's the wrong approach (though admittedly i haven't reviewed the code)
< jonasschnelli>
(It's good you haven't so I can change it before you do. :) )
< jonasschnelli>
sipa: what would you prefer then?
< sipa>
jonasschnelli: as i explained
< sipa>
refactor the current block downloading logic to support multiple users
< sipa>
make it keep track of what blocks to download
< sipa>
and what to do when they arrive
< jonasschnelli>
sipa: I thought you are going to say that... I just think this is way larger but probably cleaner
< sipa>
it's much more refactoring, yes
< sipa>
but it'll be tiny to add light client fetching on top :)
< jonasschnelli>
heh.. Yes. After that one or two year for the block download refactoring.. :)
< jonasschnelli>
I guess I focus to much on the minimal.impact solution rather then on the clean solution because I'm too much feature driven
< jonasschnelli>
ryanofsky: Not sure if the idea in that comment fulfils sipa idea of having "multiple users for the download logic"
< morcos>
sipa: i also like that idea, but i wonder if there is a priority difference there... like how quickly you time out, or find someone else to deliver or penalize for bad behavior or etc...
< ryanofsky>
because blocksToDownloadFirst is not sufficient for multiple users?
< morcos>
who me?
< morcos>
oh him
< jonasschnelli>
ryanofsky: No I think because we would only partially (out of band request) allow multiple users for the block download. But not exactly sure to what extend sipas solution should go to
< ryanofsky>
oh i think i see the difference. sipa's idea is to keep track of blocks to download and "what to do when they arrive". my idea would just do the first and let client figure out what to do when blocks come
< sipa>
what do you mean with 'out of band' ?
< jonasschnelli>
sipa: out-of-band is a poor multi-user approach for block download (the in-band is validation, the rest is out-of-band)
< sipa>
i don't understand what you mean with 'out of band' in this context
< jonasschnelli>
sipa: i guess what you meant is that the validation part uses the same block download interface then the light-client wallet? right?
< sipa>
yes
< jonasschnelli>
sipa: the validation requests block after it's own logic (not to far away, etc.), out-of-band requests can request a range of blocks that make no sense for the validation (in terms of time prioritation)
< jonasschnelli>
sipa: I guess ryanofsky terms `blocksToDownloadFirst` is much better
< sipa>
i still don't understand what you mean with out of band
< sipa>
there are blocks to download, there is logic to determine which ones to download in what order, they're processed when they arrive
< jonasschnelli>
I think i should call it "priorizedBlockRequests"
< jonasschnelli>
sipa: Yes. But the light client (wallet) wants to break that oder by a) preferring other blocks first, b) pass them through BlockConnected() before they are connected to the tip
< jonasschnelli>
(connected to the tip == validated, not connected == spv)
< sipa>
sure
< jonasschnelli>
ryanofsky: I think sipa's idea makes sense in the long run (don't distinct between wallet spv block requests and the validation triggered block request), ideally the validation would then use the same interface then the (light-client) wallet would use
< jonasschnelli>
but this would be a lot of work
< sipa>
sure, not everything has to happen in one go
< ryanofsky>
yeah, i get that now. i think that refactoring is basically orthogonal though
< jonasschnelli>
I agree...
< ryanofsky>
you could do that refactoring with or without prioritized downloads, or vice versa
< sipa>
ryanofsky: agree
< jonasschnelli>
I even think implementing the prioritized downloads gives a better feeling on how-to-refactor
< jonasschnelli>
because you refactor with a clear interface use-case
< sipa>
yes, fair enough
< jonasschnelli>
But I'm still not sure if it makes then sense to implement it within net_processing or leave it in a designated file (not very different IMO)
< jonasschnelli>
Probably most devs things within net_processing.cpp makes more sense...
< jonasschnelli>
*think
< jonasschnelli>
Then let me do that and let me rename it from auxiliary (out-of-band) to prioritizedBlockRequests
< cfields>
ok, i managed to patch boost and fix the lto linking error...
< cfields>
anyone feel like looking at a head-scratcher?
< cfields>
(and statics for each of the failures in the link)
< BlueMatt>
ah, is that why? static objects with implicitly-defined destructors?
< cfields>
so my best guess is... the static vars are created there when the lib is built, so an entry for the dtor is added. But we never use those functions...
< cfields>
so i think it's either: a gcc/lto bug, or some crazy ODR-like issue
< cfields>
oh, also, we build with fvisibility=hidden. when that's off for boost _and_ our build, it links fine
< BlueMatt>
lol
< gmaxwell>
Did Paul Sztorc talk to anyone here before publishing his thing? (except luke-jr) I want to remark on his "
< gmaxwell>
I wrote the roadmap to try to be representative of a Core / developer position." -- that if so he might have wanted to talk to some of the more frequent ones, it doesn't appear that he did.
< gmaxwell>
But I want a fact-check before saying htat.
< gmaxwell>
s/htat/that/
< BlueMatt>
i havent heard from him
< BlueMatt>
(nor speak to him specifically about that issue at any point i can recall)
< cfields>
i traced the symbol references in linker dumps. as-is, with lto, the destructor gets stuffed into util.o (because it's one of the files where there's a static reference)
< cfields>
but when i comment out the static references in util.cpp, it links fine, and the destructor goes into a boost object (operations.o)
< cfields>
IIRC when it's ambiguous, the linker is free to choose where to put the symbol, as long as it's just added to one object
< cfields>
but with lto, it gets put into util.o. then, the link-order causes boost_filesystem to be unable to find it
< sipa>
cfields: so, you fixed it?
< cfields>
i believe so
< cfields>
2 fixes: 1 quick hack to use a static string rather than a fs::path, to avoid the issue entirely. 2. the upstream boost fix