< luke-jr>
gmaxwell: hmm, but it makes sense to show them up top in the question page :/
< gmaxwell>
luke-jr: so make the order different for each, questions by least answers answers by most
< luke-jr>
gmaxwell: done
< luke-jr>
hm, it's a shame we're releasing 0.15 without bech32 sending :x
< gmaxwell>
luke-jr: when does segwit activate?
< luke-jr>
gmaxwell: Aug 21 it looks like
< gmaxwell>
luke-jr: also it's just not that mature yet,... a serious problem was found in the test vectors of the spec and ref implementations just a week ago.
< luke-jr>
:<
< gmaxwell>
luke-jr: dunno if you read the minutes from the last meeting (or was it the one before last) but we were talkign about a short cycle release with segwit wallet support right after 15 in any case.
< luke-jr>
I didn't, but newbery did mention it to me. I agree a rapid 0.16 would be good.
< luke-jr>
is there a reason to have a C++ implementation at all? seems to me the proper route to go would be to make a C libbech32, which can be used by Core…?
< gmaxwell>
a library for 50 lines of code... what are you, a nodejs developer? lpad() ftw? :P But seriously, a native C++ interface is typesafe. Wrappign a C implementation to give it a C++ friendly interface would probably end up being the same size as the library itself.
< gmaxwell>
FWIW it's much simpler to implement bech32 than base58.
< luke-jr>
hmm
< achow101>
I suspect that #10982 is going to need to be locked soon due to brigading and trolling
< jonasschnelli>
How do we deal with the service bits 6 and 8? Should NODE_NETWORK_LIMITED avoid bit6 because something else claimed it (although not via BIP process)?
< achow101>
It was actually bits 5 and 7 that were claimed
< luke-jr>
kidnapped*? :P
< jonasschnelli>
achow101: Bip 7 is SW2x, right? Whats 5?
< luke-jr>
jonasschnelli: BCash I assume
< luke-jr>
achow101: sigh, would have been nice to get the commit message fixed before merging :/
< gmaxwell>
unfortunately s2x people were spamming their slack with it and such the moment it went up.
< luke-jr>
it would also be nice if we didn't merge things because of trolls :x
< luke-jr>
jonasschnelli: perhaps: uint16_t archival_data_flag = (servicebits & NODE_NETWORK_FLAG_MASK); if (servicebits & NODE_NETWORK) { … } else if (archival_data_flag == NODE_NETWORK_LIMITED_HIGH) { … } else if (archival_data_flag == NODE_NETWORK_LIMITED_LOW) { … } would fit nicely
< MarcoFalke>
Reminder to call gpg --keyserver pgp.mit.edu --refresh-keys F4316B9B
< gmaxwell>
achow101: depends on if you count the bits starting at 0 or 1
< luke-jr>
gmaxwell: which has always been counted from 0..
< jnewbery>
NODE_NONE must surely be zero?
< gmaxwell>
depends on your language, "first bit" is 1<<0.
< jonasschnelli>
From what I read you usually start a 0
< wumpus>
doing last checks on 10882, after which I'll merge it. After that I'm planning to tag 0.15.0rc1. Any objections?
< wumpus>
(ah yes #10483 and #10607 too)
< gribble>
https://github.com/bitcoin/bitcoin/issues/10483 | scripted-diff: Use the C++11 keyword nullptr to denote the pointer literal instead of the macro NULL by practicalswift · Pull Request #10483 · bitcoin/bitcoin · GitHub
< wumpus>
then branching off 0.15, bumping version numbers, then tagging the release
< gmaxwell>
I think 10882 is still in a state where if you run getnewaddress to the pont where it exhausts your keypool then recieve a transaction your node shuts off.
< wumpus>
can you coment jnewbery?
< wumpus>
if so, i'm going to bump it to 0.15.1 instead
< jnewbery>
Yes, that's the case - I reverted to the commit that had ACKs
< wumpus>
is it enough of an improvement to merge it like this?
< jnewbery>
I think it's an improvement
< wumpus>
(or does it make things worse? I don't know anymore)
< gmaxwell>
It's a critical improvement, but I think the sequence of events I described should block a release, just like absense of this PR. Keep in mind that most existing already encrypted wallets have less than 500 keys in their keypool already.
< wumpus>
you want to block the release on this?
< gmaxwell>
I think we need to turn HD off without this. We currently have non-trivial funds loss exposure without this.
< wumpus>
is it a regression since 0.14?
< gmaxwell>
It was introduced in 0.14 with hdwallet, though we only realized over time all the ways it could fail.
< wumpus>
I'd really dislike blocking 0.15
< wumpus>
anyone else think we should delay 0.15?
< wumpus>
as I understand, the plan is to do a 0.15.1 fairly quick after it anyway
< gmaxwell>
Agreed! but I don't think it's acceptable to keep shipping stuff we know will basically make people lose funds, and yet I don't think we can ship something that will just fall over on most encrypted wallets in use right now.
< wumpus>
so every day we delay here delays that, too
< gmaxwell>
I know.
< wumpus>
and if it was already broken in 0.14, and still broken in 0.15, releaseing at least won't make things worse
< wumpus>
delaying just means people will keep using 0.14.x in which it is broken too
< gmaxwell>
whatever we do next might be fast but it's not going to be days.
< wumpus>
anyhow, going back to other things then, we can postpone for another day at least
< gmaxwell>
jnewbery: can you make a second PR with the remaining parts
< gmaxwell>
I don't understand why that fix was removed.. it's really pretty broken without it.
< jnewbery>
I plan to
< gmaxwell>
OK.
< jnewbery>
because it and the other changes broke a test, and it was suggested I revert to a commit that already had lots of ACKs
< wumpus>
jnewbery: agree that doing a new PR is better
< wumpus>
let's just merge 10882 before the review there turns even more into a mess, if some state of a PR is sufficiently accepted and reviewed ideally no new things are added
< gmaxwell>
(It's not that I don't think we can merge whats there, but I think we can't release with it because it will fail very quickly for basically anyone with a pre-existing encrypted wallet)
< wumpus>
it will fail for anyone?
< wumpus>
ok, then I'm not going to merge it, I already had half a hart attack with a 'corrupted' wallet yesterday :p
< gmaxwell>
wumpus: right now with that PR if you get a transaction while your keypool has under 500 addresses in it, you'll shut down.
< jnewbery>
yes, I realise now that the combination of 10882 and increasing the keypool defaults probably breaks this for pre-existing wallets
< gmaxwell>
jnewbery: well this was going to need a look ahead greater than 50 regardless.
< wumpus>
then why does everyone ack it if it creates a broken situation
< gmaxwell>
jnewbery: sorry for my earlier comments on it not making it adequately clear that this applies to almost everyone, I just thought that pointing out that it would go down after a getnewaddress loop was enough. :)
< luke-jr>
wumpus: my only possibly-not-ignored objection I think, is to breaking backward compatibility for RPC :/ (#10989)
< gmaxwell>
luke-jr: I think that objection is acknoweldged and set aside. Making MW require the wallet was an intentional decision at least for now that basically everyone supported.
< jnewbery>
wumpus - I think because it's been open for a long time and gone through many iterations, during which time the keypool defaults were increased. I've only just realised the implications from what gmaxwell said just now
< jnewbery>
I can hopefully get a minimal PR on top of #10882 in the next hour that includes juse the getnewaddress fix. gmaxwell, will you be able to review today?
< luke-jr>
so people will just be forced to switch to Knots to actually use MW I guess, whatever
< luke-jr>
I suppose that's the same as with previous versions
< gmaxwell>
luke-jr: come on dude, having to specify which wallet you want is not "having to use". It's perhaps arguably less useful, but the feature is expiremental in this version.
< wumpus>
multiwallet is experimental
< wumpus>
even if it's completely useless in rc1, I'm not going to block the release on that
< wumpus>
(and I know for sure it's not compltely useless)
< luke-jr>
gmaxwell: no existing software uses the new API, and as you point out, the new API is experimental.. so there's basically no way to get the primary "just keep wallets in sync" benefit without using a new experimental API
< gmaxwell>
luke-jr: bitcoin cli works, and many other things are easy to get working...
< luke-jr>
anyway, I acknowledge that this isn't going to be fixed; I'm just not happy about it.
< gmaxwell>
I agree it dimishes the wallet warming usecase.
< gmaxwell>
luke-jr: the concern people have about accidentally interacting with the wrong wallet is a good one though. It's an obvious footgun and it's more conservative to avoid it for now.
< gmaxwell>
luke-jr: also, perhaps that walletwarming usecase would better be met with a seperate state e.g. a scanwallet=file where there is no risk of accidentally using it beyond getting some status about it.
< luke-jr>
gmaxwell: yes, I agree that's a reasonable concern. I don't know a way to address it.
< luke-jr>
hopefully MW will be more than warming by 0.16 anyway
< luke-jr>
(I actually think it might have been better to release without *any* RPC MW support, than with what we have now)
< luke-jr>
(that'd solve the footgun issue, while not breaking the warming use case)
< gmaxwell>
luke-jr: fwiw, I think it would be more constructive on concerns like this if instead of just saying it's wrong, if you walked us through the specific case that you care about that it screws up. It's a lot easier to sympatize with your concern if someone understands it, and they can't unless you explain it. When you just state matter of factly that it's currently wrong or broken, it's random c
< gmaxwell>
hance if any given person will understand why you think that.
< gmaxwell>
luke-jr: For many people, updating to make the calls (e.g. cli users) is trivial.
< luke-jr>
gmaxwell: my understanding was that it's basically the only real use case 0.15 will support MW for
< gmaxwell>
and being able to actually use MW will be nice.
< gmaxwell>
luke-jr: that wasn't my understanding. Actually, I didn't even really know people were thinking that much about "warming but not using" until I read the release note text re: GUI. Which I assume you wrote.
< gmaxwell>
Though I agree it's a useful thing to do, esp in a world with pruning.. (also rescan times are a seriously negative part of my life these days)
< luke-jr>
(FWIW, Newbery wrote it)
< gmaxwell>
I manage cold wallets for myself, for blockstream, for varrious projects... and it's not uncommon that I lose hours of waiting due to having to rescan something.
< gmaxwell>
ah okay!
< jnewbery>
yeah - I think for qt-only users warming is the only use in v0.15
< luke-jr>
I suppose a reasonable compromise is to have a command-line option to set a default wallet, and only support it in that case. But probably too late to get it into 0.15
< jnewbery>
but for RPC users, multiwallet is fully supported
< luke-jr>
hmm. can a Qt user even access their wallet via debug console now?
< jnewbery>
yes, just wallet 0
< jnewbery>
#10870
< gribble>
https://github.com/bitcoin/bitcoin/issues/10870 | [Qt] Use wallet 0 in rpc console if running with multiple wallets by jonasschnelli · Pull Request #10870 · bitcoin/bitcoin · GitHub
< luke-jr>
ah
< jnewbery>
gmaxwell - I don't think the getnewaddress changes you're asking for in 10882 address the encrypted-wallet-with-fewer-than-500-keys issue that you just raised
< jnewbery>
basically every HD wallet now has fewer than 500 keys
< jnewbery>
so on upgrade, everyone with an encrypted HD wallet will have their node shut down and be presented with the 'restart with bypasskeypoolcritical blah blah blah' error
< jnewbery>
it's not a loss of funds risk, but it's a terrible first user impression of v0.15
< gmaxwell>
the whole thing is a bit weird. When the wallet is current there is no need for the keypool to have lookahead.
< gmaxwell>
But I didn't want to get into dictating major changes to it.
< wumpus>
maybe it should only enlarge the keypool the first time the user unlocks the wallet
< wumpus>
after upgrading
< Lauda>
Does anyone know what the limiting factor for rescan speed is? I have been running a few imports on a fairly good machine, and I don't see everything being utilized whilst this is taking up a lot of time (single key 2+ hours). Disk I/O maybe?
< Lauda>
CPU <20% on average, 2 GB of RAM out of 32 (with dbcache 12GB).
< wumpus>
rescan could be a lot faster if you'd not care about transaction history, then it'd only have to scan over the utxo set instead of every single block
< Lauda>
I don't, I just care about the final balance (in this case). Is there an option to ignore the TX history?
< Lauda>
Seems like rescan speed will keep getting much worse with time, no?
< jnewbery>
wumpus, the problem is that keypool_critical was changed to 500 during the iterations of this PR (when keypool was changed to 1000). We'd need a way for keypool_critical to be lowered to 50 on the first run after upgrading, or similar. I can't think of a good way to do that
< Lauda>
I should also note that it is taking over 2 hours on a VPS with an SSD. I wonder how much of a difference it would make with an HDD (% duration increase).
< gmaxwell>
the rescan is slow presumably because it deseralizes each block and each transaction and hashes each trasnaction and does dozens of memory allocations for each transactions.
< gmaxwell>
It could probably take your scriptpubkeys and txns as bytes and scan the raw block data without deseralizing things and be 500 times faster... but it would be a fair amount of work to implement that.
< gmaxwell>
jnewbery: keypool_criticial being 50 is not really reasonable in any case. it would miss transactions in many of my existing wallets, for example, and I doubt my usage is that atypical.
< gmaxwell>
jnewbery: I think the bigger point though is that when the wallet is current there is no reason to shut down.
< gmaxwell>
It doesn't matter if there is look ahead if the wallet hasn't fallen behind.
< wumpus>
yes, it will get slower with time, assuming you're donig a full rescan
< wumpus>
if you rescan a more or less fixed number of blocks it should stay at the same speed
< wumpus>
or at least not become much slower...
< wumpus>
and yes, rescan as it is could certainly be optimized, though I doubt anyone sees it as that much of a priority
< jnewbery>
"when the wallet is current there is no reason to shut down." - the getnewaddress change doesn't fix that
< Lauda>
gmaxwell that sounds like a good optimization for the future. Thanks for the responses.
< wumpus>
you're welcome to give it a try of course
< gmaxwell>
jnewbery: I realize this. When you suggested that I thought it would be simpler than another solution.
< jnewbery>
I must have misunderstood you in the issue. I suggested the getnewaddress change because I thought you were talking about running getnewaddress 500 times. I didn't realise there was an upgrade issue until you mentioned it today
< gmaxwell>
I was just trying to give an example where the keypool would be below the threshold. I don't really see them as different... end result is the same regardless of how you get there. Sorry.
< gmaxwell>
e.g. imagine that the crit threshold were still 50, you'd still have existing wallets shut down immediately, or after the user executes a dozen getnewaddresses. :)
< gmaxwell>
Just fewer of them.
< gmaxwell>
but they'd mostly all die eventually except for users that unlock frequently before running out.
< jnewbery>
ok, so how do we fix this?
< jnewbery>
Offline keypool topup would be *really* useful
< gmaxwell>
jnewbery: Can we distinguish the case where we are current vs rescanning? If so, then we just shouldn't perform the shutdown when we're current.
< gmaxwell>
I think it would still be good to stop newaddresses short of the limit, so that we don't immediately go into shutdown on any attempt to rescan when the pool is empty.. but that is a less criticial improvement.
< jnewbery>
We rescan from the wallet's best block on startup. So even with that change, we're going to barf on upgrade
< jnewbery>
(unless you shutdown v0.14, upgrade, start v0.15 before a block has been produced)
< gmaxwell>
hm. but we don't have those blocks at startup.
< gmaxwell>
when we start, the best block should be the same... though yea, IIRC we do a rescan in any case. I think just as a belt and suspenders check.
< jnewbery>
oh, ok. Makes sense
< morcos>
jnewbery: gmaxwell: i dont have all the code handy, but just brainstorming a quick fix for 0.15
< morcos>
wouldn't it be possible to take advantage of the two different limits
< gmaxwell>
I think that initial rescan can be disabled, but it's been so long since I've thought about it, there may be reasons that I am not remembering.
< morcos>
if we set keypoolmin to 500 and keypoolcritical to 25, then don't we not miss anything since our best block has stopped advancing?
< gmaxwell>
oh. that is an interesting point, but what about wallets with empty keypools
< gmaxwell>
(below 25)
< morcos>
then with the release notes we can recommend running walletpasphrase as soon as you startup so your keypool will top up to the new bigger amounts
< morcos>
oh wait, that doesnt work exactly...
< morcos>
shoot
< morcos>
also it would be potentially a problem for pruned nodes
< gmaxwell>
need a third number, but then I think it does except for but what if there isn't even 25.. and pruned.
< morcos>
i need to look at the code, but right now i actually think it is a broken state to set keypoolmin different from keypoolcritical
< gmaxwell>
morcos: If instead we just make it so that the only time this applies is when the wallet has fallen behind the node tip (or is otherwise rescanned)-- then I think that would be fine.
< gmaxwell>
You'd still get forced into topping up anytime you imported keys, moved wallets between nodes, etc.. but normal operation wouldn't run into it.
< gmaxwell>
I believe the reason we do rescanning at tip even when we're in sync (assuming we still do it) was a concern about reorgs and or the wallet not getting flushed consistently with blocks in an unclean shutdown or something like that.
< morcos>
The scary thing to me is the way we turn off setting the best block, but then potentially turn it back on if we haven't rescanned (thereby missing scanning a bunch of blocks)
< morcos>
i haven't looked at the most recent code, and maybe ryanofsky's suggestion fixed that i don't remember
< morcos>
i'm going to stop giving out ideas until i look more closely at the code again
< morcos>
but for the record wumpus, my view is it would be better to delay 0.15 (even by a week or two) to get this right
< morcos>
i think we're getting pretty well into understanding all the issues now, so would be good to fix it while it's fresh, and it also seems like a big enough problem, that would be nice to fix before release
< jnewbery>
morcos - I think ryanofsky's change is helpful. I tried including it but it broke tests, so I rolled back to a previous commit
< jnewbery>
gmaxwell - ShutdownIfKeypoolCritical() is only called in CreateWalletFromFile() and AddToWalletIfInvolvingMe(). I think your change would be to remove it from CreateWalletFromFile(), and only call it from AddToWalletIfInvolvingMe() if that function was called by SyncTransaction() (and not by ScanForWalletTransactions())
< jnewbery>
I think I know what I need to do - re-add the getnewaddress changes, re-add ryanofksy's changes about setting best block, and make the change above
< sdaftuar>
MarcoFalke: thank you
< wumpus>
MarcoFalke: you just beat me to it :)
< MarcoFalke>
Too much mail spam :(
< gmaxwell>
:-( sorry that i even commented on it at all, it just made it worse
< jnewbery>
~.
< sipa>
agree.
< * sdaftuar>
wonders what sipa's reasoning is
< wumpus>
jnewbery: maybe reverting the default mempool size increase for 0.15.0 will make this easier?
< Chris_Stewart_5>
How long do we support gcc versions? Curious for #8469
< wumpus>
Chris_Stewart_5: there's no fixed policy on that - the preference is to support all compilers unless there's a good reason not to, such as lack of c++11 support
< jnewbery>
wumpus: yes, that would resolve the upgrade issue, although I'm sure gmaxwell will argue against that
< wumpus>
sure, I just mean maybe we can't have everything in a timeframe soon enough for 0.15.0 and we need to make choices
< luke-jr>
Chris_Stewart_5: it would be pretty bad to not build on popular stable OSs
< luke-jr>
so I'd check Fedora, Debian, RedHat/CentOS, Gentoo, Ubuntu at least
< luke-jr>
IIRC typically the hold-back is CentOS
< wumpus>
luke-jr: it doesn't even work on travis
< luke-jr>
heh
< wumpus>
gcc 4.8 isn't *that* old and crappy yet, I doubt we should drop support just to be able to do tests in a certain way
< wumpus>
and the tests working on travis is kind of important, so disabling them on gcc 4.8 isn't going to fly either
< luke-jr>
right
< bitcoin-git>
[bitcoin] practicalswift opened pull request #11007: [qt] Fix memory leak when loading a corrupted wallet file (master...wallet-corrupted-leak) https://github.com/bitcoin/bitcoin/pull/11007
< Chris_Stewart_5>
Yes, 4.8.4 was released in Dec of 2014
< Chris_Stewart_5>
ancient in terms of bitcoin! :P
< sam_c>
gentoo is about to mask 4.8
< wumpus>
besides that, we already got lots of complaints that we're requiring a c++11 compiler, I"d prefer to keep the compiler requirement the same for the forseeable future
< Chris_Stewart_5>
wumpus: Do we have a 'rough' estimate on when 0.15.0 will be done?
< wumpus>
Chris_Stewart_5: yes, rc1 was planned for the 6th, we've slipped two days now
< wumpus>
(which is perfectly normal)
< luke-jr>
sam_c: is it? why?
< luke-jr>
actually, it looks like it's been masked since May, just because it's old O.o
< Chris_Stewart_5>
cool, was just curious
< jcorgan>
congrats
< AaronvanW>
+1
< arubi>
when's "status" going to change to "locked-in" ?
< arubi>
I mean, congrats! :) but also that
< gmaxwell>
oh I guess, barring no reorgs segwit lockin is now guarenteed.
< gmaxwell>
arubi: the thing happening now is that there aren't enough blocks left to prevent a lock in, but it won't lock in for another day or so IIRC.
< arubi>
so I was just informed it happens at the end of the period
< gmaxwell>
so even if all blocks for the next of the window don't signal segwit it will still lock in.
< jcorgan>
something like 16-17 hours from now
< arubi>
right, my confusion was re. the 'getblockchaininfo' "status" filed to changing after 95% rather than at the end of the period
< jcorgan>
for end of period
< Chris_Stewart_5>
Mmmm ok
< gmaxwell>
12:19:19 <@betawaffle> gmaxwell: can you congratulate everyone involved in segwit for us?
< sdaftuar>
i guess we can cross "activate segwit" off the list of action items
< gmaxwell>
\O/
< jnewbery>
gmaxwell morcos wumpus (and anyone else who's interested in wallet topup). I've pushed a branch which I think covers gmaxwell's requested logic:
< jnewbery>
- don't shutdown the node on startup if keypool is < keypool_critical
< jnewbery>
- don't shutdown the node if wallet is current
< jnewbery>
- DO shutdown the node if rescanning and keypool drops below
< gmaxwell>
When does the period end? exactly, it would kinda be nice if rc1 lands just after that, so that miners running BIP-91 code can safely try rc1.
< jnewbery>
getnewaddress should also be safe
< gmaxwell>
sdaftuar: now we just need to get people to modulate their hashrate so that the activation coincides with the solar eclipse. It's pretty close.
< jnewbery>
also includes ryanofsky's m_update_best_block
< jnewbery>
This should fix the upgrade issue. bitcoind v0.15 won't shutdown first time you run it with an old encrypted HD wallet
< jnewbery>
but if you don't unlock and topup your keypool before you shutdown the node, then I expect it'll refuse to start the next time (since your node tip will advance but your wallet best block won't)
< gmaxwell>
Right. We may need to do a bit of warning related to this pattern still:
< gmaxwell>
wallet is below 500 because it's old and encrypted.
< gmaxwell>
you start, life is good, but you are not updating the best block.
< gmaxwell>
then you restart, and now the wallet is behind.
< gmaxwell>
it rescans, and life becomes sad.
< jnewbery>
yes, sad, but better than any alternative as far as I can see
< jnewbery>
there is a warning that your keypool is below keypool_min and your best block isn't advancing
< jnewbery>
2017-08-08 19:19:20 Keypool has fallen below keypool_min (500). Wallet will no longer watch for new transactions and best block height will not be advanced.
< jnewbery>
2017-08-08 19:19:20 Unlock wallet, top up keypool and rescan to resume watching for new transactions.
< jnewbery>
Perhaps that could be made more explicit "TOP UP KEYPOOL BEFORE NEXT RESTART!"
< luke-jr>
Can we add a new checkpoint? (Minimally invasive way to lock Segwit in hard)
< gmaxwell>
luke-jr: No.
< gmaxwell>
Thats not a proper or interesting thing to do.
< luke-jr>
just to eliminate the FUD around miners potentially reorging it out
< gmaxwell>
luke-jr: you should do what we would eventually do, make a patch that removes the activation and just replaces it with a height comparison.
< luke-jr>
Isn't it too late for that in 0.15?
< gmaxwell>
probably, but we should still write the patch.
< luke-jr>
to be clear: I meant the new checkpoint specifically for 0.15.0 only
< gmaxwell>
luke-jr: but just pinning the consensus isn't a right thing to do, and especially violating the rules for setting the things.
< luke-jr>
"violating the rules for setting the things."?
< gmaxwell>
luke-jr: there is a spec for setting that requires them to be set at least 2016 blocks in the past.
< luke-jr>
it will be by the time 0.15.0 is released
< gmaxwell>
to avoid legislating the ledger state.
< gmaxwell>
also neighter of these things will prevent the reorg around the activation and steal outputs along the way.
< gmaxwell>
luke-jr: it's important to enforce these things by the time we'd issue SW addresses, less so before.
< luke-jr>
hmm, true, I guess we'd need to be checkpointing the actual activation block for that. and there isn't 2016 blocks there.
< luke-jr>
gmaxwell: Core is more than just a wallet, and wallets besides Core exist..
< sdaftuar>
while on the subject of cleaning up technical debt associated with prior soft fork deployments: #10695 could use review
< sdaftuar>
we should write the csv-height-activation patch first imo
< jnewbery>
10695 looks good to me
< sdaftuar>
jnewbery: did we ever fix the ComparisonTestFramework for the bug i mentioned in the OP of that PR?
< jnewbery>
I think we had buy-in for rewriting the ComparisonTestFramework scripts using the standard BitcoinTestFramework
< jnewbery>
In fact I've already done that for some of them, but it depends on TestNode and will several months of rebasing
< jnewbery>
*will need
< sdaftuar>
re-reading my message there though i think there was an immediate bug that makes our comparisonTestFramework tests (perhaps including p2p-fullblocktest?) not test anything.
< gmaxwell>
jnewbery: I'm still feeling this solution is incomplete. For example, one of my wallets (though not hd) is a encrypted wallet I use for watching cold funds. I'm never going to unlock the darn thing, its keypool is empty right now. Maybe what we should do for this release is merge the scan and mark as use and topup if possible but instead of shutting down, spam errors that the keypool ran out a
< gmaxwell>
nd your wallet balance may be incorrect.
< jnewbery>
we don't shutdown for non-HD wallets
< gmaxwell>
jnewbery: I know we don't, but my useage pattern wouldn't be any different if this wallet were HD.
< gmaxwell>
I still don't think we have this handling cooked, in that we'll make 0.15 unusable for at least some people or very awkward. I think we can do better even if I don't know how yet. But it's critical we do something, because without the mark use thing there are several ways to result in serious funds loss. (including giving customers addresses you already gave to other customers, then crediting t
< gmaxwell>
he wrong accounts when people make payments..)
< sdaftuar>
gmaxwell: in your watching cold funds example, would restarting with a -keypoolmin=0 be sufficient for the use case?
< sdaftuar>
(perhaps i don't quite understand the PR though, so ignore me if my question doesn't make sense)
< gmaxwell>
Yes. It is. I suppose we can just release note that. "If you want to run like this, set that." good point.
< jnewbery>
thanks sdaftuar. I think that covers greg's use case
< jnewbery>
gmaxwell can we try to evealuate whether this fixes the immediate problem (potential funds loss). We can fix up awkward behaviour post-branch
< gmaxwell>
my concern there is that we also need to not introduce something that leaves lots (?) of people stuck. But sdaftuar answers how it doesn't.
< sdaftuar>
has anyone given thought to how these keypool parameters work with multiwallet?
< sdaftuar>
eg if we advise people to use -keypoolmin=0 to solve one situation, and they load up two wallets, are they jeopardizing themselves?
< jnewbery>
yes, if you use -keypoolmin=0 you can get into a state where your best block has advanced while your keypool is depleted
< jnewbery>
but keypoolmin is a hidden debug option. I don't think we're going to be advertising it widely
< bitcoin-git>
[bitcoin] rawodb opened pull request #11009: Add information about the next state in getblockchaininfo rpc request (master...master) https://github.com/bitcoin/bitcoin/pull/11009
< jonasschnelli>
sdaftuar, luke-jr: about NODE_NETWORK_LIMITED_*. If we would treat them independently, would this mean, a peer signalling NODE_NETWORK_LIMITED_HIGH will always singal NODE_NETWORK_LIMITED_LOW?
< jonasschnelli>
And there would be no special case for a peer signaling both bits?
< jonasschnelli>
I agree that would make most sense.
< sdaftuar>
jonasschnelli: in my opinion, a peer should signal both if it offers both
< bitcoin-git>
[bitcoin] rawodb closed pull request #11009: Add information about the next state in getblockchaininfo rpc request (master...master) https://github.com/bitcoin/bitcoin/pull/11009
< sdaftuar>
that way if someone wants to write a bitcoin network client that only worries about one of those properties, they don't need to do any complicated reasoning
< jonasschnelli>
We would just loose the third state for the two new bits for pure logical consistence reasons. But I guess this makes sense.
< sdaftuar>
i would actually suggest that we rename the service bits too, if it's not too much bikeshedding... NODE_NETWORK is a non-descriptive term for full node to begin with
< sdaftuar>
and putting LIMITED in the name implies denial of "full" services, which i think isn't want we want
< jonasschnelli>
We only have 24 useful service bits (~10 are gone)
< bitcoin-git>
[bitcoin] rawodb opened pull request #11010: Add information about the next state in getblockchaininfo rpc request (master...pr/getblockchaininfo) https://github.com/bitcoin/bitcoin/pull/11010
< jonasschnelli>
sdaftuar: Agree with renaming
< jonasschnelli>
sdaftuar: any proposals?
< sdaftuar>
i guess i haven't come up with good alternatives yet, but something which means "NODE_RECENT_BLOCKS" or "NODE_REALLY_RECENT_BLOCKS" is along the lines of what i'm thinking
< sdaftuar>
(those are not actual proposals, just intuition)
< jonasschnelli>
The question is also if NODE_NETWORK implies more then just historical block responses...
< sdaftuar>
yeah that's a good question, it's not obvious to me
< sdaftuar>
ie we have non-NODE_NETWORK nodes (pruning nodes) which do all the rest, already
< jonasschnelli>
I guess there is no concrete definition what a peer must confirm to to allow signalling NODE_NETWORK (expect the reference implementation)
< sipa>
yeah, node_network just means "all the things" :)
< jonasschnelli>
also, SPV is also ~NODE_NETWORK (!= pruned)
< jonasschnelli>
~(== bit unset)
< sdaftuar>
right, but SPV is certainly allowed to eg relay addresses... not sure if any do.
< jonasschnelli>
Which results in that NODE_NETWORK_LIMITED somehow includes tx/block relay
< sipa>
i guess it's reasonable to suggest that every node can participate in address murmuring, regardless of service flags
< sipa>
it's an ootional thing regardless
< sipa>
since compact blocks, it doesn't really make sense to have separate services for tx relay and block relay
< sipa>
so the only question is whether we want to have a separate bit for relay vs historical fetch (even if it's just 144 blocks deep)
< sipa>
any client right now will want both
< jonasschnelli>
sipa: for a possible use case where a peer serves historical blocks but won't relay (pure block storage)?
< sdaftuar>
if no clients currently offer one but not the other, i don't think it makes sense to split that now. we can wait til someone wants to build that.
< jonasschnelli>
Agree
< sipa>
sdaftuar: right... the only downside is that it'd cost another bit later
< sdaftuar>
sipa: we either take that 1 bit cost now or later, right? unless there's a proposal for distinguishing now that saves a bit, which i missed?
< sipa>
sdaftuar: if we now have 3 bits (relay_tx_and_blocks, blocks_144, blocks_1008), we're good. if we only have (relay_tx_and_blocks_up_to_144, blocks_1008), we may need 2 extra bits later (relay_tx_blocks, blocks_144) totalling 4 bits, where one just implies two other ones
< jonasschnelli>
I think the current definition of NODE_NETWORK_LIMITED_LOW is:
< jonasschnelli>
NODE_NETWORK_LIMITED_LOW means the same as NODE_NETWORK with the limitation of only serving the last 288 (2 day) blocks
< jonasschnelli>
(coupled to the definition of NODE_NETWORK)
< jonasschnelli>
Changing the block value is possible though the client/protocol version?
< jonasschnelli>
If we want relay as an extra option (assume full block SPV may want to service historical blocks but not relay) another bit would be required
< sdaftuar>
sipa: i think blocks_144 and blocks_1008 without relay_tx_and_blocks are sort of nonsensical; by definition you're always willing to provide the current tip, and you never have guarantees that transactions will be relayed to you eg due to policy differences, so i don't think of that as much of a promise.
< sdaftuar>
but putting that aside:
< sdaftuar>
i think we could downgrade those service bits in the future to "may or may not relay blocks and transactions"
< sdaftuar>
if we wanted to save a bit
< sdaftuar>
oh
< sdaftuar>
i guess i'm not sure what i'm comparing...
< sdaftuar>
i'm taking jonas' proposal as relay_tx_and_blocks_up_to_144, relay_tx_and_blocks_up_to_1008
< sipa>
sdaftuar: but there is a technical difference
< sdaftuar>
and suggesting we could in the future add a single bit saying relay_tx_and_blocks
< sdaftuar>
and at the point we add that, we say that there are no promises on the quality of service you get from older nodes that aren't setting the new bit
< sipa>
sdaftuar: for example relay_blocks_tx could imply support for tx/inv(tx)/cmpctblock/blocktxn/getdata(tx), while the other implies support for getblock/getdata(block)/inv(block)
< sdaftuar>
cmpctblock and blocktxn are already separately negotiated
< sipa>
but not advertized
< sipa>
(right?)
< sdaftuar>
right
< sipa>
they're implied by NODE_NETWORK + sufficient protocol version
< sdaftuar>
well they're implied by NODE_SEGWIT, for all practical purposes
< sdaftuar>
but interesting point
< sdaftuar>
i'm not sure it's reasonable for service bits to keep up with p2p protocol improvements
< sipa>
agree
< gmaxwell>
I'm not enthused by all these flags because in general we should not be segmenting up the topology if we can avoid it.
< sdaftuar>
i agree with that. i also generally want to punt on features we are not sure anyone will implement.
< sipa>
agree
< sipa>
just pointing out that it may cost us an extra bit in the future - perhaps that's ok
< jonasschnelli>
Should peers avoid serving blocks above the NODE_NETWORK_LIMITED_LOW|HIGH threshold to reduce finger printing opportunities?
< jonasschnelli>
Assume you have pruned down to 10k blocks and signalling NODE_NETWORK_LIMITED_HIGH, one could fingerprint by checking how deep down you can serve blocks
< sipa>
yes, i think so
< jonasschnelli>
Okay. Will update the BIP
< jonasschnelli>
Or maybe just the implementation... need to check
< bitcoin-git>
[bitcoin] rawodb closed pull request #11010: Add information about the next state in getblockchaininfo rpc request (master...pr/getblockchaininfo) https://github.com/bitcoin/bitcoin/pull/11010
< Murch>
sipa: Transaction overhead is 10 bytes for P2PKH transactions, since segwit adds flag and marker, which however go into the witness part am I correct to calculate the weight of the tx overhead as 42?
< Murch>
For a transaction that spends at least one P2SHP2PWSH input
< sipa>
Murch: looks right to me
< sipa>
mryandao: yes, will do if i find the time
< Murch>
thank you :)
< sipa>
mryandao: achow101 beat me to it :)
< achow101>
:)
< bitcoin-git>
[bitcoin] gmaxwell opened pull request #11011: [Trivial] Add a comment on the use of prevector in script. (master...201708-prevector-comment) https://github.com/bitcoin/bitcoin/pull/11011