< wumpus>
now maiiz is doing the same in #8355, either this is a github issue or people are trolling us...
< paveljanik>
maiiz probably has a problem with github web interface.
< GitHub190>
[bitcoin] NicolasDorier opened pull request #8356: Wallet: Minimum output value depends on fee instead of minTxRelayFee (master...wallet-min-output) https://github.com/bitcoin/bitcoin/pull/8356
< jonasschnelli>
wumpus: no. 8308 is more or less a feature and can go into 0.14
< jonasschnelli>
IMO it's unclear if "importwallet" should import the HD master seed and set it according to the importet wallet
< wumpus>
thanks, I agree
< jonasschnelli>
*import* somehow implies that "old stuff is unchanged"
< jonasschnelli>
But its good to have the extended master key "xpriv" visible in the dump... but as said. can go into 0.14
< wumpus>
indeed, it's something that needs to be considered carefully, youd don't expect to switch to another HD chain automatically when importing say, an old wallet
< wumpus>
the only time you would want that is if you intend the imported wallet to replace your current one
< wumpus>
this would be a change going forward, for the future, it can't be done retrospectively for versions that are already released :)
< wumpus>
in any case no hurry
< sipa>
wumpus: i guess it can be replaced with a set of strings
< jonasschnelli>
Yes. We should slowly decouple it from CLIENT_VERSION
< sipa>
wumpus: denoting features that the wallet uses
< jonasschnelli>
sipa: you mean set of string about what features it supports?
< jonasschnelli>
ok.
< wumpus>
sipa: and then exit if a non-supported feature is used, yes
< jonasschnelli>
Flags? Bitmask?
< wumpus>
strings are slightly better in this case because you can report the name of the feature even in versions that don't support it
< wumpus>
e.g. generating errors like 'this wallet uses BIP32 which is not supported in this version'
< jonasschnelli>
ah.. fair enought
< jonasschnelli>
We could do this once we switch over to a different database format (logdb)
< wumpus>
with bitmask you can only report the flag number, and there's no real need to be very compact here
< jonasschnelli>
(which is overdue since years)
< wumpus>
(it'd be something stored only once in the wallet)
< sipa>
wumpus: i like the ext2/3/4 compatibility system
< sipa>
wumpus: with 3 sets of strings/flags
< sipa>
1) "if you don't know one of these, ignore" 2) "if you don't know any of these, only open read-only" 3) "if you don't know any of these, refuse to mount"
< jonasschnelli>
sipa: Indeed. This would be good.
< jonasschnelli>
But the problem is, such stuff should had be implemented at the beginning. Now it should/must be tied to a more radical wallet format change.
< sipa>
no, you can just see the switch to such a new system as a 'feature' in the old compatibility system
< jonasschnelli>
sipa: Yes. Indeed. Possible migration path.
< wumpus>
bah, changing all occurences of 'cost' to 'weight' is a huge change
< wumpus>
I'm having second thoughts about it
< wumpus>
changing it just in user-facing messages is doable, but it also appears in the RPC API, in variable names, in tons of comments
< wumpus>
and don't really want to block the 0.13 branch on this
< sipa>
so you want to delay the change until 0.13 is branched off?
< sipa>
or reconsider entirely?
< wumpus>
I don't think I want to do it anymore
< MarcoFalke>
What about only changing -help and rpc calls?
< MarcoFalke>
And then the nasty code cleaup for 0.14?
< wumpus>
I don't think it's worth it
< wumpus>
should have paid more attention to this during the BIP draft
< wumpus>
maybe just change the help message to "Set maximum BIP141 block cost"
< wumpus>
then everyone can look it up if they want
< wumpus>
changing the BIP now because of a word impacts all other implementations too
< GitHub191>
[bitcoin] MarcoFalke opened pull request #8358: [doc] gbuild: Set memory explicitly (default is too low) (master...Mf1607-docBuild) https://github.com/bitcoin/bitcoin/pull/8358
< GitHub7>
[bitcoin] laanwj opened pull request #8359: mining: Improve `-blockmaxcost` help message (master...2016_07_blockmaxcost_doc) https://github.com/bitcoin/bitcoin/pull/8359
< jonasschnelli>
Is there a way to get the best known height? I just figured out, that NotifyHeaderTip() will not pass it the best known height header..
< jonasschnelli>
I expected that the main logic will get all headers first and pass them through NotifyHeaderTip() before actually downloading blocks...
< sipa>
yes, it will
< sipa>
it always notifies with the best known header at the time of calling
< jonasschnelli>
sipa: okay. Let me check again....
< jonasschnelli>
sipa: but it seems it loads a couple of headers, then the according blocks and not all headers to the best known height. Right?
< sipa>
jonasschnelli: i cannot parse your sentence
< jonasschnelli>
okay.. I'll try to explain different.
< jonasschnelli>
1.) Im connected to a node with height 421290, I'm currently at 421100
< jonasschnelli>
2.) NotifyHeaderTip gives me a header somewhere at height 421150 (50 headers in advance)
< jonasschnelli>
3.) Then I'll get the signals for the blocks till 421150
< sipa>
sounds correct so far
< jonasschnelli>
Is there no way to get signal with "the best known header" (from the remote peers)?
< jonasschnelli>
(or the best known height)
< jonasschnelli>
I want to work on the UI progress
< sipa>
you don't know the peer's best header until we have it as well
< jonasschnelli>
sipa: and main does not load all headers first,.. only a bunch of them, then the blocks?
< sipa>
exactly what do you want to do?
< jonasschnelli>
I'd like to show the "remaning blocks to process" untill we are in sync
< sipa>
show the difference between the height of the last notifybesttip and the last notifybestheader
< jonasschnelli>
So... I'd like to get signaled with "all" the headers first
< sipa>
that's what notifybestheader does...
< jonasschnelli>
sipa: I do that.. but its not really what i wanted. Say I'm 300 blocks behind. The GUI now report 50 blocks behind, slowly reduces that number, jumps back to 50, etc.
< sipa>
if it's behind a lot the headers should progress much much faster than the blocks
< sipa>
it usually takes only a minute or two to learn about all headers
< sipa>
ah!
< sipa>
notifybestheader is not called during IBD
< sipa>
oh, no, it is
< sipa>
but with a false parameter
< jonasschnelli>
ah... IBD is also true if I start bitcoin-core with 200 blocks behind. Right?
< sipa>
yes
< sipa>
but it is always called
< jonasschnelli>
Hmm.. let me debug more deep
< sipa>
i'm surprised it only increases 50
< sipa>
headers should increase by 2000 at a time
< jonasschnelli>
Maybe I'm missing the first signal because of my implementation... sipa: could it be that i get older headers in later NotifyHeaderTip()?
< sipa>
at the time of the notify, what you see is always the best known header
< jonasschnelli>
RPC getblockchaininfo report a newer height that I get passed over NotifyHeaderTip()
< sipa>
during IBD that's possible
< sipa>
oh, wait, no
< sipa>
are you not filtering out IBD NotifyHeaderTips somewhere?
< jonasschnelli>
It works much better if i initially call pindexBestHeader->nHeight; then only "accept" header height over NotifyHeaderTips() if they are >
< jonasschnelli>
Although not sure if this works for reorgs
< sipa>
i'm very confused
< sipa>
where in that code you're showing me do you hook into the notification?
< sipa>
ah, you're adding to setNumBlocks
< sipa>
that's only called once every 250ms
< jonasschnelli>
ahh.. that could be the problem
< sipa>
the header reported through NotifyBestHeader should always be of increasing height
< sipa>
unless there is a very deep reorg to a lower height
< sipa>
but that's extremely unlikely
< jonasschnelli>
Argh.. yes. There is something like: if (!initialSync || now - nLastUpdateNotification > MODEL_UPDATE_DELAY) {
< jonasschnelli>
Thanks sipa!
< jonasschnelli>
we could pass through the fHeader=true signal always...
< luke-jr>
wumpus: just think of how much more trouble it will be to change s/cost/weight/ *after* 0.13 gets released, if we end up back at that :/
< sipa>
luke-jr, wumpus: i'm also a bit surprised by how large you fear the impact of the change is... the term cost (in externally-facing code) is only for blocks, not transactions
< sipa>
though i didn't try it myself, so i may be missing things
< luke-jr>
sipa: well, at least changing it in GBT will be annoying once cost is released
< sipa>
ah, GBT
< sipa>
i hadn't considered that we'd change things there as well
< sipa>
but i guess, yes
< luke-jr>
I guess we don't *have* to, but it will just be confusing if GBT was the odd thing out
< sipa>
agree
< jtimon>
yay branch 0.13 forked !
< sdaftuar>
hmm, it seems that "sigops cost" isn't explicitly defined in bip 141 (though i guess it's implied), yet we refer to that term in the output of gbt
< sdaftuar>
we didn't talk about it last week, but is "sigops cost" a term we want to keep?
< luke-jr>
sdaftuar: it was renamed to just "sigops"; I'm surprised GBT ever mentioned it O.o
< sdaftuar>
" \"sigops\" : n, (numeric) total SigOps cost, as counted for purposes of block limits; if key is not present, sigop cost is unknown and clients MUST NOT assume it is zero\n"
< luke-jr>
oh, in Core; that's just a bug then IMO
< sdaftuar>
it seems strange to me that we'd choose to redefine a term...? at the least we'd have to say "total BIP141-sigops" or something
< sdaftuar>
but that seems clunky!
< luke-jr>
sdaftuar: why?
< luke-jr>
sigops are just counted differently, not redefined
< sdaftuar>
it's a different unit now
< luke-jr>
P2SH counted them differently without a rename too
< luke-jr>
the old counting ceases to be relevant at the fork
< sdaftuar>
p2sh didn't change the units/scale of sigops for non-p2sh transactions though
< luke-jr>
…
< sdaftuar>
i don't think we can just multiply everything by 100 and not change the name, even if mathematically it's the same effect
< luke-jr>
so you want to rename it every softfork that affects how it's counted?
< luke-jr>
what's the point?
< sdaftuar>
luke-jr: i see that getblocktemplate also returns a "sigoplimit" which is explained as "(numeric) cost limit of sigops in blocks", assuming we change that language as well, is there any reason a gbt caller would care what scale the sigops counting is being done in?
< sdaftuar>
if not then fine, i agree this is all pointless
< luke-jr>
sdaftuar: a GBT caller cannot know the sigop counting; scale is not different from any other countign changes
< sipa>
sdaftuar, luke-jr: imho we should change the name when the unit's scale changes
< luke-jr>
sigh.
< sipa>
luke-jr: in p2sh, users of non-p2sh transactions would not be affected by the new counting
< sipa>
here, everyone is affected - assuming that the old unit has the same meaning could lead to accidents
< luke-jr>
how?
< sipa>
that's why transaction's "size" is redefined in a backward-compatible way (and not the scaling)
< sipa>
i agree that for sigops it is unlikely to matter
< sipa>
but in general, it is a good practice to rename things when their meaning changes
< luke-jr>
IMO the meaning is essentially the same.
< luke-jr>
it's an arbitrary consensus-critical counting toward an arbitrary limit.
< sipa>
if someone were to charge based on #sigops, it is most definitely not the se
< sipa>
*same
< sipa>
do you agree that if we'd apply the same logic to size/cost, peoplr would be in trouble, because they may pay 4x too high a fee? (if they apply a feerate measured in bytes to a cost variable)?
< sipa>
i was fine with not renaming in GBT, but instead giving the consensus limit along with it
< luke-jr>
yes, because size is something they can calculate external to the consensus system
< luke-jr>
the same is not true of sigops - you can't calculate it independently from the UTXO set
< sipa>
i don't think that's relevant
< luke-jr>
(specifically the UTXOs you're spending)
< sipa>
it's a consensus-constrained resource
< sipa>
its usage may affect fees
< sipa>
this has traditionally not been the case for sigops, so i think it is unlikely to matter
< sipa>
but in general, i don't think we can apply the argument "it needs UTXOs to calculate, thus it is safe to arbitrarily rescale"
< sipa>
those two properties are completely unrelated
< luke-jr>
if you can't calculate it in the first place, then you can't try to do a sigop-feerate independently
< GitHub116>
[bitcoin] sdaftuar opened pull request #8362: Scale legacy sigop count in CreateNewBlock (master...coinbase-sigops-scale) https://github.com/bitcoin/bitcoin/pull/8362
< wumpus>
luke-jr sipa I just think there's more important things to focus on than changing words now
< wumpus>
I thought it was simply a matter of changing that option, but I severly underestimated things
< wumpus>
and as horrible names go, I don't think anything beats 'segregated withness' itself :)
< sdaftuar>
wumpus: i've started to make an attempt at the change myself. agreed that it's a bit tedious! but i think if we're willing to consider merging, it'd be better to clear the language now than be saddled with it indefinitely
< wumpus>
my experience is that people will get used to any term, but if you really think pushing on with it makes sense I'm not opposed to it
< wumpus>
at some point I got to GetTransactionCost and stopped bothering
< wumpus>
(did I really need to change that to GetTransactionWeight?)
< sdaftuar>
i think yes
< sdaftuar>
:)
< sdaftuar>
anyway i will try to wrap up and see what it looks like
< sdaftuar>
sipa: what is your preferred language for the new sigop metric, if you have one?
< sdaftuar>
i guess the existing language, not in the BIP, is "sigop cost"
< luke-jr>
can we just leave sigops alone? unlike cost/weight, at least sigops becomes a complete non-issue over time :x
< wumpus>
I was almost afraid that people would complain the unit needs to be Kg after this change :-)
< luke-jr>
wumpus: srsly?
< wumpus>
no, not really, but GetTransactionWeight sounds weird to me too, though it's lots better than GetTransactionCost
< jtimon>
wumpus: lol then maybe some would prefer the imperial system...
< sdaftuar>
right, GetTransactionCost sounds like fee!
< luke-jr>
"impact" may work as well.
< wumpus>
agree sdaftuar
< btcdrak>
jtimon: ack on imperial measurement.
< luke-jr>
jtimon: tonal !
< sipa>
sdaftuar: i don't care strongly about sigops
< sipa>
and cost is inconsistent, as the other is not called "size cost" either
< sdaftuar>
i think we could just add "sigops cost" as a term in the BIP, and be done with the issue
< luke-jr>
I think "BIP 141 sigops" would make for a reasonable compromise since it makes it easy to drop "BIP 141" in the future
< wumpus>
there's also at least one other implementation of segwit that will have to be changed with the BIP141 change (the btcd implementation)
< Chris_Stewart_5>
So weight is signifying the subsidy segwit txs receive right?
< luke-jr>
Chris_Stewart_5: no, weight is the replacement for size limits
< Chris_Stewart_5>
luke-jr: size limits of..?
< luke-jr>
of prior versions of Bitcoin
< luke-jr>
it used to be that each byte of the block counted as 1 byte toward a 1 MB limit
< Chris_Stewart_5>
So this is modifying the Script program size, tx size, block size...?
< luke-jr>
now we count bytes as different "weights" toward a 4,000,000 limit
< Chris_Stewart_5>
oh, ok
< luke-jr>
more expensive data affecting the UTXO set have a weight of 4 per byte
< luke-jr>
less expensive data (witness scripts) have a weight of 1 per byte
< Chris_Stewart_5>
luke-jr: At the risk of asking a stupid question, why is this needed? Shouldn't segwit txs literally be smaller than old txs? Why have this artificial multiplier
< sipa>
Chris_Stewart_5: they're not smaller
< sipa>
where did you get that idea
< sipa>
segwit is a block size increase
< Chris_Stewart_5>
When segwit txs are serialized and sent over the network they don't include scripts, making them smaller?
< GitHub50>
[bitcoin] f139975 opened pull request #8364: Fix counting of sigops cost in mempool check (master...fix-mempool-sigops) https://github.com/bitcoin/bitcoin/pull/8364
< arubi>
sipa, and what if a miner does it? wrt ^
< sipa>
arubi: what if a miner does what?
< arubi>
well, I guess I'm asking if creating large legacy sigops blocks will be easy for a miner in case the sigops limit is high
< sipa>
i don't understand
< sipa>
arubi: the problem that #7081 fixed was that the mining code produces very suboptimal blocks when there are high-legacy-sigops transactions in the mempool
< sipa>
#7081 fixed it by refusing transactions that have high sigops but low size
< sipa>
i think the correct solution is just to treat those transactions as if they had the corresponding size
< arubi>
sipa, I think I understand. the reason I'm interested is because I've experienced this on segnet4. broadcasting bare multisig txs was impossible without setting bytespersigop=1
< arubi>
it was confusing, something like a 1-of-16 would go through, but not a "sane" 1-of-2 or 2-3, iirc. I eventually just set bytesper..=1 and forgot about it
< arubi>
and you mentioned a vulnerability (which you explained), so.. hence the first question :)
< BlueMatt>
jtimon: its an obvious change (current time is context by most definitions) which removes one more #include for the single CheckBlock call in blockencodings.cpp
< BlueMatt>
(that one line adds like 4 deps, at least)
< sipa>
longer term i think CheckBlock and ContextualCheckBlock can probably be merged, as we never validate a block anymore without having its context
< sipa>
though some parts of validation need to be factored out... for consistency checking and the verification compact blocks need
< GitHub89>
[bitcoin] sipa opened pull request #8365: Treat high-sigop transactions as larger rather than rejecting them (master...unifysigopcost) https://github.com/bitcoin/bitcoin/pull/8365
< BlueMatt>
sipa: yea, I mean not a bad idea, indeed
< BlueMatt>
but, yea, need to pull out the mutation-possible checks
< jtimon>
BlueMatt: well, yeah, i guess strictly is context, but any caller can have time. In contrast, not all callers may have access to the the block index. If we were to expose checkblockheader and contextualcheckblockheader in libconsensus separately maybe some SPV callers use checkblockheader but not contextualblockheader...anyway, I guess we can move it back later when we're talking about exposing things
< jtimon>
maybe exposing verifyheader (calling both) is enough and we don't care where the check is for this
< BlueMatt>
jtimon: I mean the only thing that the compact blocks stuff cares about is the IsCorruptionPossible() checks
< BlueMatt>
jtimon: those need to be factored out to simplify the compact block code anyway...everything else, whatever
< jtimon>
anyway, not important at the moment I think, I was just surprised to see it moved
< GitHub175>
[bitcoin] jonasschnelli opened pull request #8366: [Wallet] Ensure <0.13 clients can't open HD wallets (0.13...2016/07/hd_minversion) https://github.com/bitcoin/bitcoin/pull/8366
< GitHub57>
[bitcoin] jonasschnelli closed pull request #8343: [Wallet] Ensure <0.13 clients can't open HD wallets (master...2016/07/hd_minversion) https://github.com/bitcoin/bitcoin/pull/8343
< GitHub83>
[bitcoin] jonasschnelli opened pull request #8367: [Wallet] Ensure <0.13 clients can't open HD wallets (master...2016/07/hd_minversion_014) https://github.com/bitcoin/bitcoin/pull/8367