< bitcoin-git>
[bitcoin] jnewbery opened pull request #18808: [net processing] Drop unknown types in getdata (master...2020-04-getdata) https://github.com/bitcoin/bitcoin/pull/18808
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #18809: rpc: Do not advertise dumptxoutset as a way to flush the chainstate (master...2004-rpcNoFlushAdvert) https://github.com/bitcoin/bitcoin/pull/18809
< sipa>
would it be a problem to increase that to 1.59?
< fanquake>
What's the reason for doing so? I'll recheck all the distros and see if that'd be an issue
< fanquake>
CentOS 7 seems to ship with 1.53
< sipa>
i'm looking into using a multi_index for keeping track of tx requests, and it seems 1.59 adds "ranked indices", which would avoid the need for some separate things to keep track of
< sipa>
multi_index is headers-only, so it'd only be a build dependency, not a runtime one
< lucaferr>
Has mempool p2p command been deprecated? The information I can find online indicates it should reply with an inv, however in integration tests it seems to disconnect. Is it only working for SPV clients as of now?
< bitcoin-git>
[bitcoin] promag opened pull request #18814: rpc: Relock wallet only if most recent callback (master...2020-04-rpc-wallet-relock) https://github.com/bitcoin/bitcoin/pull/18814
< bitcoin-git>
[bitcoin] rebroad closed pull request #18760: Disconnect nodes with bad messages during IBD (master...IBDDisconnectBadChecksum) https://github.com/bitcoin/bitcoin/pull/18760
< vasild>
wumpus: While looking at the addrv2/BIP155 wip PR #16748 I realized that in CSubNet we support netmasks that have 0-bits followed by 1-bits, e.g. /255.255.0.255.
< dongcarl>
vasild: I apologize for not responding in time to the reviews, it's on my list now :-)
< vasild>
I am just wondering - why do we support such strange (invalid) netmasks and can we drop that support?
< vasild>
dongcarl: no problem, I got distracted by the netmasks and dived into some CSubNet cleanup that would drop support for netmasks like /255.255.0.255.
< dongcarl>
vasild: I'm guessing you're thinking just have a uint type that specifies the prefix length?
< vasild>
dongcarl: you read my thoughts!
< dongcarl>
Hmm... well are there legitimate cases where a netmask can have 0's in the middle?
< vasild>
replace the `uint8_t netmask[16];` in CSubNet with `uint8_t cidr;` which can be in [0, 32] for ipv4 and [0, 128] for ipv6.
< vasild>
dongcarl: right, I do not know, but to me that contradicts with the idea of subnetting
< vasild>
I was thinking that this is just some unintended side effect, but then found a deliberate test cases that expect such support (above link ^), so I decided to ask :)
< dongcarl>
vasild: I think it'd probably be good to leave as is, mostly because the contiguous-ness is not really enforced in the RFCs, and it seems more flexible for the future. Will look into it though.
< vasild>
"Subnet masks were allowed by RFC 950[5] to specify non-contiguous bits until RFC 4632[4]:Section 5.1 stated that the mask must be left contiguous. Given this constraint, a subnet mask and CIDR notation serve exactly the same function."
< bitcoin-git>
[bitcoin] practicalswift closed pull request #17895: build: Enable Clang's -Wconditional-uninitialized to warn on potential uninitialized reads (master...continue-killing-of-uninitialized-reads) https://github.com/bitcoin/bitcoin/pull/17895
< vasild>
this modification fiddles with the `uint8_t netmask[16];` array
< vasild>
and having to increase that to netmask[32] would be far from perfect, given that it can be just a single byte.
< dongcarl>
vasild: Working on sth else right now but will take a look today. One thing that came to mind when I last looked into this: the NetMask code looks like it was written with the assumption that all CNetAddrs would be addresses in address families where netmasks made sense... With the addition of tor, that's not the case anymore
< vasild>
Right! Netmasks do not make any sense in the new types too: torv3, i2p, cjdns.
< bitcoin-git>
[bitcoin] practicalswift closed pull request #18147: qt: Kill the locale dependency bug class by not allowing Qt to mess with LC_NUMERIC (master...kill-locale-dependency-bug-class) https://github.com/bitcoin/bitcoin/pull/18147
< bitcoin-git>
[bitcoin] practicalswift closed pull request #18124: init: Clarify C and C++ locale assumptions. Add locale sanity checks to verify assumptions at run-time. (master...LocaleSanityCheck) https://github.com/bitcoin/bitcoin/pull/18124
< vasild>
I will leave the "uint8_t netmask[16]" -> "uint8_t cidr" change away for now. Ideally we shouldn't be using netmasks for e.g. torv3 at all, or we just use one special mask which denotes "matches just one address"
< dongcarl>
vasild: Sounds good, always good to explore the options :-) I'll take a closer look at your comments today
< vasild>
dongcarl: btw, I rebased the PR on latest master (required some non-trivial conflict resolution) and fixed the failing unit tests.
< dongcarl>
vasild: Oh yeah? Do you have a branch up?
< vasild>
dongcarl: not pushed on gh, I was wondering how to proceed with that...
< wumpus>
agree, netmasks make no sense for non-hierarchical addresses types
< dongcarl>
vasild: Well, if it's acceptable to you, you should put yourself as co-author on the commits, and push as a branch to your remote on github e.g. `vaslid/bitcoin`, and I'll get those commits into my branch/PR
< wumpus>
and the cidr-based notation is more compact, though it might be more expensive to match against? I'm not sure
< vasild>
dongcarl: makes sense, I will clean up a bit and push to gh.
< dongcarl>
vasild: Many thanks!
< vasild>
yw :)
< vasild>
wumpus: more compact and the only valid representation for ipv6, as for speed of matching - we have to compare the first `cidr` bits, I don't think it will make a difference wrt performance.
< vasild>
btw our current implementation https://github.com/bitcoin/bitcoin/blob/master/src/netaddress.cpp#L815 is checking all 16 bytes unnecessary - it could stop checking (break; from the loop) as soon as it finds a 0 byte in the netmask. E.g. with a netmask like /255.0.0.0 we don't need to check whether `(addr.ip[x] & netmask[x]) != network.ip[x]` is true or false for the last 3 bytes
< wumpus>
yes, I'm fairly sure it can be optimized to be just as fast or even faster, without converting to an and-mask inbetween
< wumpus>
no need for constant time matching here
< vasild>
this is all minor, low-priority I guess
< wumpus>
correct
< wumpus>
though some people have extremely large ban lists :)
< vasild>
how large?
< wumpus>
~1000 entries
< vasild>
hmm
< wumpus>
not something any CPU capable of running a bitcoin node in the first place will have any trouble with I guess, it's only per-incoming-connection not per packet
< bitcoin-git>
bitcoin/master 66fe7b1 Harris: test: added test for upgradewallet RPC
< bitcoin-git>
bitcoin/master e302830 MarcoFalke: Merge #18774: test: added test for upgradewallet RPC
< vasild>
seems like a minor / low priority performance improvement can be done in the subnet matching code wrt that. Also the size of CSubNet would be reduced with 15 bytes, 15*1000 bytes is nothing in RAM or on disk
< achow101>
does anyone know what nWalletMaxVersion is for? and why it's distinct from nWalletVersion
< achow101>
I think we should be able to consolidate these. which would also make upgradewallet less confusing
< phantomcircuit>
it seems at some point in the (probably distant) past the "settings" key for the wallet db was called "setting"
< phantomcircuit>
which is now causing the unknown key counter to go up
< phantomcircuit>
however i cannot find any evidence of when that was
< sipa>
settings like the position and dimension of the UI screen were once upon a time stored in the wallet
< phantomcircuit>
there's a "settings" key which is only "known" to avoid the unknown flag, but no "setting" key
< phantomcircuit>
sipa, interesting
< phantomcircuit>
this is probably irrelevant to like 99.99% of people since this wallet is a mega old testnet wallet
< sipa>
heh
< sipa>
i wonder if settings got turned into setting in some refactor and nobody ever noticed
< sipa>
or the other way around
< phantomcircuit>
sipa, possibly only in master and not a release even
< sipa>
heh
< luke-jr>
achow101: upgradewallet doesn't actually do the upgrade until the new features are needed
< luke-jr>
so it increases max version immediately, and the actual version gets bumped no higher than that on demand
< sipa>
yes, exactly
< achow101>
luke-jr: would it be reasonable to break that with the new upgradewallet rpc? That kind of makes sense for the -upgradewallet option, but for a RPC, the upgrade is explicitly being done so bumping the version regardless seems reasonable to me
< sipa>
it used to be the case that upgradewallet would permit the use of compressed keys, but only once you actually created a compressed key was the wallet marked as a new version
< sipa>
that mechanism wasn't used for encryption, where encrypting would automatically and immediately upgrade iirc
< achow101>
this nWalletMaxVersion thing is also only being applied to FEATURE_WALLETCRYPT and FEATURE_COMPRPUBKEY which were introduced ages ago
< sipa>
if that mechanism isn't used anymore it may make sense to get rid of it
< sipa>
compressed keys was 0.5 iirc
< sipa>
in 2011...
< achow101>
it only makes sense if you expect the user to downgrade after doing an upgradewallet. and that really doesn't make sense with upgradewallet now being a rpc as its even more explicit
< luke-jr>
achow101: frankly, I'm stuck on 0.13 for my wallet because I dislike the direction the wallet is going.. so I'm not sure my opinion counts.
< luke-jr>
also, our efforts to keep backward compatibility without upgradewallet have failed anyway.. so
< achow101>
also there seems to be a nasty bug in SetMinVersion if your wallet was older than FEATURE_WALLETCRYPT and then you encrypted it, the version would jump to the latest. That's a problem as the wallet would have the indications to be HD without actually being HD
< luke-jr>
(eg, inclusion of segwit txs in old wallets)
< luke-jr>
achow101: encrypting the wallet would make it HD tho, no?
< achow101>
luke-jr: maybe?
< achow101>
I was just looking at SetMinVersion and this looked like it could be a huge problem as it was just unconditionally jumping to FEATURE_LATEST