< instagibbs>
BlueMatt in not too long it will be 5 digits...
< sipa>
at which point we'll need a bot even more
< nanotube>
BlueMatt: that can be easily arranged.
< GitHub141>
[bitcoin] gmaxwell opened pull request #9052: Use RelevantServices instead of node_network in AttemptToEvict. (master...prefer_relevant2) https://github.com/bitcoin/bitcoin/pull/9052
< GitHub58>
[bitcoin] gmaxwell opened pull request #9053: IBD using chainwork instead of height and not using header timestamps (master...no_checkpoint_for_ibd) https://github.com/bitcoin/bitcoin/pull/9053
< gribble>
https://github.com/bitcoin/bitcoin/issues/9053 | IBD using chainwork instead of height and not using header timestamps by gmaxwell · Pull Request #9053 · bitcoin/bitcoin · GitHub
< gmaxwell>
haha
< nanotube>
haha well... i guess gribble should ignore github >_>
< phantomcircuit>
gmaxwell: int64_t &nOrderNextPos = nOrderNextPos in CWallet::ReorderTransactions is trying to assign a reference to itself not the member variable
< nanotube>
^ that happens :)
< phantomcircuit>
i dont get this
< phantomcircuit>
do you?
< sipa>
phantomcircuit: ha
< sipa>
phantomcircuit: yes, variables are defined during their own initializer
< phantomcircuit>
ok then i understand what was wrong with #8828
< gmaxwell>
sipa: would there be any benefits beyond saving a few bytes of source code for each of the couple uint256 constants we have?
< sipa>
no
< cfields_>
sipa: mm, i coded that up at one point
< cfields_>
gmaxwell: it's especially nice because you can initialize on the stack more quickly (and efficiently)
< sipa>
cfields_: it's just an invocation to a conversion operator, instead of a function
< cfields_>
(you can parse a variadic of hex chars as constexpr, so the parsing is free)
< sipa>
i'm pretty sure there's no difference at runtime
< sipa>
oh!
< sipa>
well we could also make uint256S constexpr then?
< cfields_>
sipa: well, it only works for literals
< gmaxwell>
I think in all cases they get stashed in constants, so it shouldn't really make a runtime difference.
< cfields_>
yea, in reality it's negligible. I just did it to play with the variadics + user literals. But it made chainparams look prettier :)
< cfields_>
i'll see if i can dig it up
< sipa>
cfields_: right, but i guess you could at least have a uint256constS function, which is constexpr and does the same thing as the normal uint256S function
< sipa>
which you could only pass literals or other constexpr strings
< sipa>
and compared to that, a user-defined literal is purely syntactic sugar?
< cfields_>
sipa: sure, i suppose so
< gmaxwell>
https://github.com/bitcoin/bitcoin/pull/9053 < can someone trigger a rerun here? I'm pretty sure this change couldn't make test_bitcoin fail (and it passes locally.)
< sipa>
gmaxwell: chainActive.Tip()->nChainWork may segfault before the genesis block is loaded
< sipa>
almost all platforms fail, that doesn't look like a spurious error
< gmaxwell>
sipa: on #1 fair comment, but they're failing in test_bitcoin prevector tests.
< gmaxwell>
Perhaps we should as nano to change it to PR#xxxx
< sipa>
or we adapt
< gmaxwell>
FWIW, there is a bot that does this in #xiph run by someone no one knows, that just spends all its time banned. esp because it links to some trac instance thats hardly used.
< rebroad>
jonasschnelli, what does the test command do please?
< jonasschnelli>
rebroad: a test. :)
< rebroad>
jonasschnelli, ah, thank you. most enlightening
< jonasschnelli>
sipa: how do we detect nodes delaying (or even not sending) requested blocks? Currently I'm debugging a case where I have a couple of blocks "inflight" but the peer is responding extremely slow...
< jonasschnelli>
Is there a max timeout or something similar until we release the block from the mapBlocksInFlight?
< jonasschnelli>
and re-request it from a different peer?
< gmaxwell>
jonasschnelli: yes, we will eventually hang up on the peer after 10 minutes, see BLOCK_DOWNLOAD_TIMEOUT_BASE
< jonasschnelli>
gmaxwell: thanks. Also just stumbled over state->nDownloadingSince
< jonasschnelli>
gmaxwell: for my hybrid-SPV-PR this seems to be to long...
< jonasschnelli>
I guess there are no other full-block SPV wallets out there... so hard to say what would be best in a such case...
< gmaxwell>
why is that any different for hybrid-spv-pr than normal operation?
< jonasschnelli>
Yes. Same for normal operation.
< jonasschnelli>
If blocks are dropping in in a ~10min interval, the UX of an SPV mode is almost broken..
< gmaxwell>
It should be made adaptive. It cannot just be made shorter without making the software dysfunctional for hosts with a slow connection. (as it will get 95% through transfering, then abort, and start over, ... but even slower because the prior transfer left some incoming traffic...)
< jonasschnelli>
I guess during full-validation. people tend to be more patient.
< gmaxwell>
jonasschnelli: what?
< gmaxwell>
are you forcing in order block processing or something?
< gmaxwell>
normally during sync the timeout isn't the mechenism that deals with slow peers, the stalling mechenism is.
< jonasschnelli>
gmaxwell: I'm working on an SPV modes where the wallet can specifically request (priorize) a block-sequence
< jonasschnelli>
"Normal" IBD interrupts, FindNextBlocksToDownload prefers specific request
< gmaxwell>
And it doesn't have any tricky and failure prone hard timeouts, it punts peers that stall the transfer.
< gmaxwell>
so the good performance of other peers is the test rather than a static timeout.
< jonasschnelli>
The SPV modes works mostly okay, but sometimes I encountered that I have connected to extremely slow peers (block response take up to 5mins), which makes the user-experience really inconvenient.
< gmaxwell>
this is handled by the initial block download logic. It fetches blocks out of order, and when a peer is slower than all the others thus stalling things, it gets kicked off.
< jonasschnelli>
Haven't fully analyzed "nNow > state.nDownloadingSince + consensusParams.nPowTargetSpacing * (BLOCK_DOWNLOAD_TIMEOUT_BASE + BLOCK_DOWNLOAD_TIMEOUT_PER_PEER * nOtherPeersWithValidatedDownloads)"... but it seems to not trigger a 5 min delayed block around the tip
< gmaxwell>
the timeout is 10 minutes. (at the tip, normally we expect things to be transfered via BIP152 high bandwidth mode, which can't stall)
< gmaxwell>
(well my recollection is 10 minutes, and if it is set statically less then 10 minutes you will have hosts with enough bandwidth to successfully use the system constant dos attacking themselves.)
< jonasschnelli>
Okay. Thanks.
< jonasschnelli>
BIP152 doesn't helps in the SPV mode (UTXO set not ready)
< gmaxwell>
UTXO set? you mean mempool.
< jonasschnelli>
Yes. But no UXTO set no mempool. :)
< jonasschnelli>
Also not sure, but could it be, that the parallel block download during the time, when we initially sync the headers could slow down the header sync?
< jonasschnelli>
As soon as we have the first header batch, we start download those blocks (which is fine in normal operation), but in SPV, I'd prefer to get the headers with priority.
< gmaxwell>
the header sync is very fast, and we do not use much bandwidth early in the sync due to the many round trips needed due to small batches. So I doubt it does.
< gmaxwell>
in any case, as I said before... the timeout can be made dynamic. It just needs to start at a safe number like ten minutes, and then when a block transfers faster than 10 minutes, it can be reduced to e.g. 2 sec + 2*(block_time*1m/block_size) or such, and any time it kicks off a peer due to a failure, double again with a max of 10 minutes. This way it will not have a timeout so slow that it will
< gmaxwell>
cause failure, and if the node's bandwidth goes down, it will adapt back after a couple failures.
< gmaxwell>
that kind of adaptive threshold just wasn't very important so far because of the combination of other methods.
< phantomcircuit>
jonasschnelli: hybrid-spv-pr?
< phantomcircuit>
wat
< jl2012>
sipa: I got a question from a block explorer dev. How do we want people to present bare segwit on explorers?
< jl2012>
I never thought of that. Maybe using the hex?
< jonasschnelli>
phantomcircuit: Yes. Will PR soon. Need to write a RPC test first,... which is non-trivial
< jonasschnelli>
I guess you are familiar with the idea of SPV during IBD/sync (= hybrid SPV)
< GitHub45>
[bitcoin] ryanofsky opened pull request #9058: Fixes for p2p-compactblocks.py test timeouts on travis (#8842) (master...fix-8842-sync-timeouts) https://github.com/bitcoin/bitcoin/pull/9058
< phantomcircuit>
jonasschnelli: ah yeah
< phantomcircuit>
for the wallet only right?
< jl2012>
sipa: for block explorer, I think they should display bare segwit with P2SH addresses. Just like how they display P2PK as P2PKH address
< NicolasDorier>
jl2012: nop
< NicolasDorier>
it confuses new devs
< NicolasDorier>
best is just to show 0 <hash>
< jl2012>
or just the hex
< NicolasDorier>
yeah or just the hex
< NicolasDorier>
lost quite a lot of time explaining to noob why the address of the genesis output was not generating the same ScriptPubKey than inside the block
< morcos>
cfields_: I got a bit lost trying to follow you, BlueMatt and sipa discussing moving and const members, but I don't think any of that discussion changes the gains I was seeing from your copy-move branch?
< morcos>
What do you think about actually PR'ing that. I've been using it for so long now for all my benching, I'd like to make sure I'm not building my work on a house of cards
< sipa>
what does his copy-move branch do?
< gmaxwell>
jl2012: showing some other address would cause loss of funds.
< morcos>
theuni/bitcoin/copy-move
< gmaxwell>
jl2012: people would sends funds to those addresses and because they're not the same, wallets would ignore those payments.
< jl2012>
gmaxwell: yes, i think that's not a good idea. But core is still showing P2PK as P2PKH
< sipa>
morcos: casting a const field to non-const is undefined behaviour
< gmaxwell>
yes, legacy behavior but at least a ubiquitous one.
< morcos>
sipa: if i recall cfields was thinking it wouldn't show a big speedup, but it definitely makes a noticeable difference for me.
< jl2012>
ok, i told them to use raw hex directly
< sipa>
morcos: we currently do it in CTransaction, but I have a PR to get rid of it
< morcos>
sipa: you're referring to the move operator for CTransaction (or whatever thats called) in the 2nd commit?
< morcos>
i don't think I'm using that though anywhere in the code right. i didn't make any changes to explicitly use that. i'm assuming its the other changes that are causing the speedup, so i don't need that part
< morcos>
i just briefly tried putting an assert(false) in there and it worked just fine...
< sipa>
morcos: oh, making prevector movable is fine!
< sipa>
i missed there were two commits
< morcos>
it wasn't clear to me if any of the other minor changes in the 2nd commit mattered, i didn't bother trying to track down what was important...
< morcos>
pehraps the thing holding cfields_ up from actually PR'ing it is you can't use is_trivially_copyable
< morcos>
and maybe he wanted some protection for that
< sipa>
is_trivially_copyable is certainly fine for the current use cases
< morcos>
no i mean its not supported everywhere, for instance in gcc 4.8 or whatever i have on ubuntu 14.04
< sipa>
that's something you wonder about, or an issue you know?
< morcos>
yeah it didn't compile for me until i commented those lines out
< sipa>
afaik is_trivially_copyable is just c++11, so every compliant compiler should have it
< gmaxwell>
sipa: it's libstdc++ though, not GCC itself.
< morcos>
what are those called? traits or attributes or whatever, are the one part of c++11 that gcc 4.8 didn't fully support
< sipa>
well we can temporarily define our own, for just the types we use it for
< morcos>
so in your PR to fix CTransaction, you don't define a move constructor right? would it be benfecial to have one? or is it useless since the CTransaction it's moving from has all members const?
< morcos>
thats where i got lost in yoru conversation the other day
< sipa>
right, the move constructor would be identical to the copy constructor
< sipa>
and my suggestion is that we solve it at a higher level: if a ctransaction needs to be movable, use a std::unique_ptr or std::shared_ptr around it
< morcos>
which is inefficient though huh, since isn't the whole point of the move that you're not going to use the source object any more
< sipa>
in particular in CBlock
< morcos>
btw, if you have any brilliant ideas, i've been spending a lot of time trying to figure out how to flush pcoinstip but keep it warm
< morcos>
you can do something semi-smart in regular operation by constructing fake blocks (although usually there isn't enough of a mempool backlog for this to be too helpful)
< morcos>
but what in the world do you do during startup...
< morcos>
gmaxwell: no i mean when you're in IBD or something (so its not that you don't have a mempool, its that you're connecting blocks from long ago)
< gmaxwell>
"don't do it then"
< morcos>
it's sad how long HaveInputs takes in a reindex-chainstate or similar operation
< sipa>
morcos: even with huge dbcache?
< sipa>
because HaveInputs is what triggers the fetch from disk if an entry is not cached
< morcos>
sipa: no thats what i mean. with a reasonable sized one
< morcos>
i'm wondering if when you go to flush it, you can be smart about keeping some useful stuff in there...
< sipa>
perhaps we should experiment with per-output caching rather than per-tx
< morcos>
sipa: i just worry that without changing the whole database it'll be hard to use that efficiently
< sipa>
oh, sure, the database will need to change
< sipa>
but it can be converted on startup or so
< morcos>
well you could imagine a middle ground.. but yeah i agree, cleaner to just do the whole thing
< sipa>
the one thing i'm not clear on is how to deal with the (few) per-tx values
< morcos>
well, we should stop storing nVersion
< morcos>
i think what we store for that is undefined actually
< sipa>
agree, i doubt we'll need it
< morcos>
i was trying to trace it yesterday, it looks like we right shift a signed int to store it as a CVarInt which I belive is implementation dependent
< sipa>
indeed
< morcos>
there is a comment about it not properly handling negative numbers already
< sipa>
so if it's just height and coinbaseness, we could perhaps have just coinbaseness stored in every utxo
< sipa>
and a separate map with remembered heights of coinbase tn
< morcos>
you need the height for more than coinbase though right?
< sipa>
do we?
< morcos>
bip68?
< sipa>
oh, with bip68 we do
< sipa>
ok
< sipa>
maybe it's easier to not bother with deduplicating the height then
< morcos>
certainly easier!
< morcos>
so will the txid be duplicated on disk though for each utxo?
< morcos>
i assume its easy enough to skip that in memory, but not sure how leveldb works, that can we have two levels of keys?
< sipa>
i believe that leveldb efficiently represents may keys with the same prefix
< sipa>
*many
< sipa>
but i can't find a reference for it
< instagibbs>
does the rpc tester eventually time out? It's hanging on me and I can't tell which test is triggering the timeout
< sipa>
bigtable (the internal system at google that leveldb is an opensource simplified version of) certainly does that
< sipa>
morcos: an alternative is two tabbles, where one maps txid to (coinbase,height,id), and id is a 64-bit sequentially incrementing value, which is used in a (id,nout) to (scriptPubkey, amoubt) map
< sipa>
downside: twice the number of disk seeks
< morcos>
sipa: yeah i 2 disk ops is going to be worse for a lot of disks
< sipa>
or we can of course leave the disk format intact
< gmaxwell>
So, re. prefinal alert, right now all those old nodes are displaying "Warning: This version is obsolete, upgrade required!". It would be a bit odd to override that message with something merely saying the alert system is going away.
< BlueMatt>
"The Alert System has been deprecated. Please upgrade your client"
< BlueMatt>
?
< gmaxwell>
What I wrote before checking to see the language of the current warning was "Alerts are retired and removed in current software."
< gmaxwell>
BlueMatt: your suggestion suggests that alerts are the only reason to upgrade.
< achow101>
How about "Upgrading is strongly recommended" instead of "Please upgrade your client"
< achow101>
won't both the alert and the other message be displayed simultaneously?
< gmaxwell>
It's weird to override a more serious warning with a less serious one.
< gmaxwell>
achow101: no, they can't be.
< gmaxwell>
achow101: the messages have a priority and the highest priority active alert is displayed.
< gmaxwell>
if this is sent at low priority (which was what I was originally intending before I thought about it) then it just won't be seen by anyone because it would be masked by that upgrade required alert.
< achow101>
What about two different messages, one for those who are showing that upgrade required alert, and one not?
< gmaxwell>
AFAIK, there aren't any nodes which display alerts which aren't showing upgrade required. Anything prior to 0.12.2 is displaying upgrade required right now. (and once segwit signaling starts, all 0.13 will also start displaying an upgrade notice)
< BlueMatt>
sipa: we're all getting emails about old releases :(
< BlueMatt>
bitcoin 0.10.4 released
< sdaftuar>
time to upgrade!
< achow101>
gmaxwell: so just make the message "Warning: This version is obsolete. The Alert System has been deprecated. Upgrade is strongly recommended. See: https://bitcoin.org/alert-retirement"
< sipa>
BlueMatt: oops
< sipa>
BlueMatt: i was just adding release notes to tags
< arubi>
genuinely asking, why not recommend to run with -alerts=0 from now and that's it?
< gmaxwell>
achow101: makes it sound like the alert system is the only issue, but it's not-- the reason they're saying upgrade required is that they're not current with the current network rules.
< gmaxwell>
arubi: uh oh. confusion detected.
< arubi>
is that a different alert then?
< gmaxwell>
arubi: alert=0 has been the case for many versions, and since 0.13.0 the alert system is gone entirely.
< arubi>
yea but you were talking about 0.12.2
< gmaxwell>
arubi: that has alert=0 by default.
< arubi>
I see, "prior to"
< TD-Linux>
gmaxwell, just concatenate the messages? "In addition, the alert system..."
< achow101>
how long can the message be? You could make it more explicit that the software is both out of date with consensus rules and that the alert system is deprecated
< gmaxwell>
achow101: I think it'll get cut off in the tray of the application in older versions if it's too long. But otherwise there isn't a meaningful limit.
< achow101>
so, how about "Warning: Network rules in this version are out of date. Additionally the Alert System has been deprecated. Upgrade is strongly recommended. See https://bitcoin.org/alert-retirement"
< arubi>
gmaxwell, to be sure, "upgrade required" is a message invoked locally when rule changes are detected, and "alerts" are simply the messages that come in signed by the alert key? sorry for getting in the way here
< achow101>
maybe have a "due to soft forks" after "rules out of date"
< kanzure>
high amounts of meail from recent releases on github
< kanzure>
.. email.
< sipa>
kanzure: i'm sorry, i didn't know it was mailing people
< achow101>
gmaxwell: if the message is an issue now, what about when "Alert Key Compromised" is sent?
< gmaxwell>
arubi: yes, -- the local message and alerts are handled in exactly the same way.
< gmaxwell>
achow101: it's even more urgent though, and also less of an issue since we'll have months of this message being displayed.
< achow101>
ok. What do you think about my other suggestion?
< arubi>
thanks gmaxwell, I'll have to play around with alerts= with my own mock alert key
< gmaxwell>
achow101: sounds fine to me more or less.. Perhaps "Warning: This is outdated and network-inconsistent software. Also, the alert system has been deprecated. Upgrade is strongly recommended. See https://bitcoin.org/alert-retirement"
< achow101>
sounds good to me.
< achow101>
how do you even send an alert?
< gmaxwell>
achow101: using software bitcoin's creator wrote which has never been publically distributed.
< phantomcircuit>
gmaxwell: possibly just send the same message
< phantomcircuit>
iirc there are very very old versions which wont show that warning
< gmaxwell>
yes, though AFAICT all of those are forked off. :)
< gmaxwell>
we want to mention the alert system specifically, so that when the alert system compromised message shows up it might be a little less alarming.
< sipa>
cfields_: no, have a constexpr function that takes a hex string... if the string has length 0, return 0. if not, call recursively, multiply by 16 and add the new hex digit
< sipa>
cfields_: oh wait, uint256 no longer has operator* and operator+
< cfields_>
sipa: right, just dumb bits
< sipa>
cfields_: so ignore what i said
< cfields_>
sipa: roger. doing it in 64bit chunks would probably be cleaner, though