< phantomcircuit>
wumpus, replaced that wallet test with a python one
< phantomcircuit>
i think it actually covers more of the behavior than it did before but im going to go and check
< phantomcircuit>
hmm im not testing addmultisigaddress
< phantomcircuit>
but that's already being tested in other places
< phantomcircuit>
although not that it works with accounts
< wumpus>
phantomcircuit: nice!
< wumpus>
sipa: the last update of secp256k1 has been ~9 months ago (6c527ec), is it maybe time to update the subtree again? At least the (experimental) ARM assembly has been merged since
< sipa>
wumpus: good idea
< sipa>
there are certainly no regressions known that would make the current master less appropriate than the subtree in bitcoin currently
< wumpus>
exactly, I did mean for master, not 0.13
< sipa>
sure
< GitHub30>
[bitcoin] laanwj opened pull request #8453: Bring secp256k1 subtree up to date with master (master...2016_08_update_secp256k1) https://github.com/bitcoin/bitcoin/pull/8453
< GitHub71>
bitcoin/0.13 d485a6c Wladimir J. van der Laan: doc: Add list of new and removed RPC commands to release notes...
< wumpus>
sipa: do you still plan on writing something for the release notes about -reindex-chainstate?
< wumpus>
I've added a list of added and removed and changed RPC calls, which means that #7678 can almost be closed, leaving -maxtimeadjust and -reindex-chainstate, of which I think reindex-chainstate (and the changed reindexing in general) is most relevant
< GitHub160>
[bitcoin] pedrobranco opened pull request #8457: Add block height support in rpc call getblock (master...feature/add-get-block-by-number) https://github.com/bitcoin/bitcoin/pull/8457
< sipa>
wumpus: ack, will write
< instagibbs>
during salvage on an unbroken wallet.dat file I'm seeing keys getting skipped
< instagibbs>
on master
< instagibbs>
0-4 keys on a fresh wallet with keypool 100, randomly
< gmaxwell>
I've said before that salvagewallet should be called "savagewallet".
< gmaxwell>
instagibbs: can you figure out why?
< instagibbs>
no ive been trying to chase it down all day
< instagibbs>
it's very random, and only happens once every 25+ keys
< gmaxwell>
presumably you've filled up the salvage code with instrumentation to see where they're getting dropped? (e.g. does bdb just never hand them to us?)
< instagibbs>
it's an error thrown when doing >> for private key
< instagibbs>
"key" type, trying to deserialize the key
< instagibbs>
priv*
< sipa>
is it trying to deserialize master key data as the wrong type?
< instagibbs>
I don't think so, as keypool=1 almost never has it happen
< instagibbs>
I've never had it happen with keypool 1, that is to say
< wumpus>
instagibbs: can you upload the wallet somewhere that it happens with?
< instagibbs>
it's literally a fresh wallet, start up bitcoind in regtest, stop, start back up with salvagewallet. Some not small % of time i get those messages
< instagibbs>
but yes I can upload one
< wumpus>
it's a known issue btw: salvagewallet drops valid keys #7379 but I've never had it happen to me
< wumpus>
another similar problem is salvagewallet fails verification #7463 , salvagewallet on an uncorrupted wallet sometimes makes bdb return errors
< wumpus>
so what you are seeing may be similar, I noticed that problem, just never saw keys being dropped
< instagibbs>
2016-08-04 18:07:42 Salvage(aggressive) found 439 records
< wumpus>
I would not be surprised if bdb's "aggressive" salvage sometimes returns crap or misses records
< instagibbs>
oh is it an actual bdb mode? That might explain the difference
< gmaxwell>
maybe it should run twice, once normally without any bdb magig, and then again in agressive mode?
< gmaxwell>
"never do worse than simply reading it."
< wumpus>
I don't know if that makes a difference, but worth a try
< instagibbs>
i can try that, if i figure out the settings
< gmaxwell>
well presumably it must, after all we don't miss those keys during normal reads without salvaging.
< wumpus>
ooh! those may be ghost keys instagibbs
< wumpus>
"Output all the key/data pairs in the file that can be found. By default, DB->verify() does not assume corruption. For example, if a key/data pair on a page is marked as deleted, it is not then written to the output file. When DB_AGGRESSIVE is specified, corruption is assumed, and any key/data pair that can be found is written. In this case, key/data pairs that are corrupted or have been deleted may appear in the output (even
< wumpus>
if the file being salvaged is in no way corrupt), and the output will almost certainly require editing before being loaded into a database."
< instagibbs>
i aint afraid of no ghost
< wumpus>
aggressive should return *more* records, but some of them may be deleted, or not really exist at all
< cfields>
sipa: mm, I just noticed that 0.13 doesn't enable asm for osx due to secp256k1 #373 :\
< wumpus>
*even if the file being salvaged is in no way corrupt* is the important part there
< instagibbs>
so how can we test that
< wumpus>
so yes, trying to salvage without aggressive makes sense first
< wumpus>
but how can we detect if keys have been dropped due to corruption, which would appear with salvage aggressive? I don't know
< wumpus>
yes you don't really know, that's the problem
< wumpus>
was it a real key or just a corrupted echo?
< wumpus>
cfields: maybe do the secp256k1 update for 0.13.1 too then
< instagibbs>
so worst case with "ghost" keys would be having keys you didn't actually generate? or?
< wumpus>
instagibbs: or partial old records
< cfields>
wumpus: probably makes sense, yes. I'm running a bench with/without atm.
< wumpus>
instagibbs: it does a linear scan over the file and everything that looks like a key/value is returned
< instagibbs>
so aggressive will indeed return more, but we toss anything that looks wrong
< paveljanik>
Trivial -Wshadow PRs for review: 8109, 8163, 8191, 8449. I'd be glad to get some ACKs. Thank you.
< wumpus>
instagibbs: yes, we do, we're fairly robust there. But it means possible false positives inthe log
< instagibbs>
wumpus, ok, well that's less scary, thanks
< wumpus>
instagibbs: I hope that's it! but you can only find out by analysing that wallet\
< wumpus>
see if the keys that are in the original wallet and post-salvage wallet are the same, despite the WARNING
< sipa>
wumpus: i'll merge the strings include in secp
< wumpus>
sipa: thanks
< sipa>
so that can be included in the subtree
< wumpus>
right, I'll update the pr
< cfields>
116us vs 124us. not the end of the world.
< wumpus>
don't think that'd be worth it for any random warning, but missing prototypes are pretty serious business
< sipa>
cfields: that's the output of bench_verify ?
< gmaxwell>
cfields: I'm surprised by that.
< gmaxwell>
the difference is smaller than I expected.
< sipa>
likewise
< cfields>
sipa: yes
< sipa>
what compilation flags?
< cfields>
sipa: default passed down from bitcoin
< wumpus>
116 versus 124 seems in the error margin, are you sure something changed at all?
< gmaxwell>
sipa: that looks broken, those numbers are much higher than I expect.
< gmaxwell>
(for both of you)
< sipa>
that's without gmp or endomorphism, at 2.6 GHz
< cfields>
gmaxwell: remember no bignum or endo
< gmaxwell>
I normally expect a verify to be more like 70us. but maybe thats all gmp.
< cfields>
jonasschnelli: thanks
< wumpus>
MarcoFalke: please don't merge anything for 0.13 now, we have to decide on the meeting whether to release final, this is a test change but I don't want to have to trigger a new rc without good reason
< MarcoFalke>
This should not be enough for a new rc
< wumpus>
I know, but you named it 0.13.1, and it's not time to merge for 0.13.1 yet, just clarifying
< MarcoFalke>
oops
< * gmaxwell>
predicts a build system problem.
< gmaxwell>
(in libsecp256k1)
< wumpus>
(people may still want to do e.g. changelog changes before tagging final)
< gmaxwell>
cd ~
< cfields>
gmaxwell: for which?
< wumpus>
bitcoin is always without gmp
< sipa>
gmaxwell: sounds likely... with endo and gmp enabled is see no statistically significant performance difference between asm enabled and disabled
< sipa>
71.2us vs 71.4us
< sipa>
(with variance between tests exceeding 0.2us)
< gmaxwell>
Aside, I think in the GUI and logs we should display the verification performance... so people will better realize how radically difference in speed different machines are. :)
< wumpus>
yes, a graph of verification performance in the gui would be nice
< wumpus>
similar to the bandwidth one
< wumpus>
with part spent in i/o secp256k1 etc
< jonasschnelli>
heh.. currently working on a mempool graph
< luke-jr>
I find this double-standard somewhat annoying.
< morcos>
sigh, if this is another meeting of us all arguing against luke-jr then i'm going to find something else to do
< jonasschnelli>
heh. Yes. Finish this. luke-jr can open a pr (action)
< wumpus>
I'm not arguing against luke-jr
< wumpus>
anyhow that derailed my question - so no one had any critical problems with 0.13.0rc2?
< wumpus>
and I don't mean with the release notes
< sipa>
is there any fear of instagibbs' problem being due to a code error?
< wumpus>
what problem?
< instagibbs>
sipa, fwiw it's showing up on older fork...
< instagibbs>
it's not HD-related, or anything fairly recent
< gmaxwell>
instagibbs: so without the bip32 changes?
< instagibbs>
correct
< wumpus>
you mean the salvagewallet problem? that's old hat, there's two issues open with salvagewallet problems
< sipa>
instagibbs: ok, good to know
< wumpus>
as I mentioned
< sipa>
sorry, i didn't follow the whole discussion
< wumpus>
this is not a last minute problem
< luke-jr>
wumpus: earlier we were discussing 0.13.0's failure mode downgrading from 0.13.1; I am not clear what the status of that is, or if we need changes for it
< wumpus>
yes, we've caught up a bit with the rc1 slip by doing a fast rc phase
< GitHub107>
[bitcoin] luke-jr opened pull request #8458: [0.13] release notes: remove bad advice to switch to blockmaxweight prematurely (master...0.13_relnotes_remove_bad_advice) https://github.com/bitcoin/bitcoin/pull/8458
< btcdrak>
0.13.1 shouldnt be too far behind at least, but my preference is to include it and do another rc
< wumpus>
note that I planned amonth for the rc1 phase
< wumpus>
eh for the rc phase
< wumpus>
well the downgrade protection cna't be postponed to 0.13.1
< wumpus>
that'd defeat the point
< wumpus>
otherwise that'd be my preference
< luke-jr>
8458 also mentions an idea I had for a 3-liner to make blockmaxsize perform identical to blockmaxweight, if we do another rc.. if that's desired, someone please say so and I'll do it now
< sipa>
luke-jr: i think you included a few commits too many
< wumpus>
yes now everyone starts with last minute ideas
< wumpus>
great
< luke-jr>
oh crap, I made it against master
< GitHub168>
[bitcoin] luke-jr closed pull request #8458: [0.13] release notes: remove bad advice to switch to blockmaxweight prematurely (master...0.13_relnotes_remove_bad_advice) https://github.com/bitcoin/bitcoin/pull/8458
< gmaxwell>
luke-jr: nah, something changing transaction selection.. not a good thing now.
< luke-jr>
wumpus: well, it's trivial and probably unnecessary; fine if we don't do it
< wumpus>
#action check if downgrade protection is really needed, or whether it already fails
< GitHub10>
[bitcoin] luke-jr opened pull request #8459: [0.13] release-notes: Do not encourage changing blockmaxsize to blockmaxweight (0.13...0.13_relnotes_remove_bad_advice) https://github.com/bitcoin/bitcoin/pull/8459
< wumpus>
other topics?
< cfields>
oh
< sipa>
segwit mempool malleability dos (#8279)
< cfields>
(sec, someone else feel free to go ahead)
< wumpus>
#topic segwit mempool malleability dos
< wumpus>
wasn't that supposed to be solved in #8312?
< sdaftuar>
no, it was only solved for 0.13.0
< sdaftuar>
we need to decide what to do in our first segwit release
< sipa>
so i suggested removing the "transaction failure purely due to witness does not cause rejection cache entry"
< sipa>
with the rationale that all it does it prevent an attacker from hiding a valid transaction from you in some cases
< sipa>
but it doesn't prevent it entirely (they can announce and just never send the transaction)
< sipa>
sdaftuar notes that the mempool malleability attack only needs a short lived connection, while the never send tx data attack needs a persistent one
< sdaftuar>
and it needs more than one persistent one -- you need several, as we retry tx download from other peers
< sipa>
sdaftuar: rejection cache is also temporary, but a node won't just re-request...
< morcos>
i think i'm coming around to the idea of full verification of all txs
< gmaxwell>
morcos: hah
< BlueMatt>
was there rationale to inv'ing with txid instead of wtxid for segwit nodes?
< BlueMatt>
(just noting that wtxid would be Keep The Same Behavior As Pre SegWit (tm))
< sipa>
BlueMatt: duplicating a lot of logic (mempool, orphan, caches, ...), and causes at least a potential doubling anyway (you could be inved the same tx from a pre-segwit and post-segwit node once with txid and once with wtxid, without being able to tell they're the same)
< gmaxwell>
also in the long term, the high malleability of witnesses would let an attacker waste lots of bandwidth. with nodes relaying different witness versions of the same txn among each other.
< BlueMatt>
sipa: if you're inved a tx with a witness from a pre-segwit node we get a double anyway since its garbage to us
< BlueMatt>
gmaxwell: same is true for scriptSigs...up to IsStandard limits (which could, in fact, be more easily enforced on witnesses)
< sdaftuar>
BlueMatt: that's true but shouldn't happen, as pre-segwit nodes shouldn't be accepting segwit transactions unless they're running with an unusual policy
< sipa>
also, if you have a tx with malleable witness (say, op_drop argument), nodes on the network could modify the witness without invalidating, and you wouldn't even be able to tell you already had the transaction before download
< sipa>
and you couldn't ban them for it, because they're all valid transactions
< gmaxwell>
BlueMatt: the kind of isstandard restriction against malleability only works great for limited classes of transactions, at least in the long term that isn't great.
< BlueMatt>
I'm aware
< sipa>
a solution is inving with both txid and wtxid... but if we go that way, we should also adds resource information to all invs (fees, size, sigops, ...)
< BlueMatt>
sipa: I was about to say that...seems like a real solution is inving both...
< morcos>
aren't we approachign the size of the tx at that point
< sipa>
morcos: certainly within an order of magnitude
< BlueMatt>
gmaxwell: but IsStandard is allowed to serve the same purpose as always - enforce reasonable limits to ensure code sanity until we've explored edge cases thouroughly
< gmaxwell>
the txid is within an order of magnitude. :P
< sdaftuar>
sipa: i think we might we just end up overfitting current policy rules by adding resource information
< gmaxwell>
(for the median size)
< sipa>
sdaftuar: 'overfitting' ?
< gmaxwell>
but future mempool sync these sizes will matter less.
< gmaxwell>
sdaftuar: feerate is pretty fundimental, however.
< sdaftuar>
ie policy will change, and the information will be insufficient
< gmaxwell>
I think including sigops would be dumb.
< sdaftuar>
gmaxwell: yes, but already handled with less bandwidth by feefilter
< gmaxwell>
if sigops really matter, something is wrong.
< gmaxwell>
sdaftuar: indeed. feefilter makes it implicit.
< sipa>
well, ok... we could send size and fee
< sipa>
but that's not much better than making feefilter mandatory
< sipa>
except slightly more flexible
< sipa>
and sending two hashes is annoying
< gmaxwell>
we're in the weeds for this right now.
< sipa>
agree
< sipa>
i think there is no simple clear cut solution for this
< gmaxwell>
if we're doing set recon it doesn't really matter that much how long the identifiers are.
< sipa>
but that's much further out
< BlueMatt>
if inv'ing both hashes werent stupid for bandwidth, Id say that was a pretty great solution
< BlueMatt>
true
< gmaxwell>
sipa: depends on how long it takes me to convince you to implement the relevant number theory code. :P
< gmaxwell>
In any case, the witnessID really doesn't matter for much of anything except rejection here.
< sipa>
alternative idea: make feefilter mandatory for segwit, and just disable rejectioncache...
< sipa>
rejectioncache only helps in practice against repeated announcements of transactions below your threshold
< sipa>
it's not a protection against attacks
< sdaftuar>
hm, not a bad idea
< luke-jr>
feefilter isn't even used by default last I knew? (because there is no min fee until the mempool fills)
< sipa>
luke-jr: good point
< BlueMatt>
damn, mem^H^H^Hdiskpool was too late :(
< morcos>
i'm all for making fee filter mandatory, if i'd known people would have liked it this much, we should have designed it that way from teh beginning
< morcos>
i'm a bit worried about removing the rejection cache entirely
< gmaxwell>
luke-jr: there is a minrelayfee.
< gmaxwell>
Do we not feefilter on that?
< sdaftuar>
gmaxwell: but we let in free transactions still
< gmaxwell>
oh right.
< morcos>
anytime there is any policy difference between nodes (such as a change in minrelay fee change isDust) then you get txs bouncing aroudn the network between the sets of nodes with different policies
< gmaxwell>
full validation and distinguishing why rejection happened would help.
< morcos>
i think its nice to have a sort of protection against that
< gmaxwell>
we could just have one rejection filter per peer type.
< gmaxwell>
e.g. rejection filter for non-sw peers and a rejection filter for sw peers.
< sdaftuar>
with sw peers using wtxid invs?
< sipa>
sdaftuar: still not enough... you need to be able to tell whether you already have another version of the same tx
< sipa>
even with only sw peers
< gmaxwell>
+full validation.
< sdaftuar>
sipa: i don't see why that's needed, any more than we have today with not knowing a tx is a double-spend before processing
< sipa>
sdaftuar: someone connects to a 1000 nodes on the network, and relays a different version of the same valid transaction to each
< sipa>
sdaftuar: now you potentially need to download 1000 transactions
< sipa>
only one of which pays fees
< morcos>
proposal: make feefilter mandatory. fully validate txs so we can punish peers who send us invalid stuff. don't put any witness tx in the rejection cache. then evaluate how useful rejection cache continues to be or whether we have policy violating segwit txs bouncing around
< sdaftuar>
so you mean a 3rd party uses signature malleability to do this?
< sdaftuar>
because the tx originator can already do that
< sipa>
sdaftuar: right, the distinction isn't all that big
< sipa>
morcos: i like that... but i think it's a big change for 0.13.1
< gmaxwell>
the distinction though is that an attacker doing it pays fees, eventually runs out of funds, vs an attacker doing it to other peoples' txn does not ever run out of funds.
< morcos>
sipa: i think if we start with " don't put any witness tx in the rejection cache" then we'll be ok
< sipa>
morcos: ack
< morcos>
we can see how easy and short he other 2 are
< wumpus>
that concludes the topic for this meeting?
< wumpus>
any other topic proposals? cfields?
< NicolasDorier>
for information, I created ##libconsensus bikeshedding room about how to handle libconsensus for anybody interested.
< cfields>
NicolasDorier: haha, i was just about to paste a blurb for that
< NicolasDorier>
aha I just woke up for that, will go back bed :D
< instagibbs>
I have to leave now, but just letting you know salvage is finding lots of "extra" keys vs keys actually in wallet. *shrug*
< cfields>
NicolasDorier and I discussed libconsensus a bit this weekend. We're hoping to discuss amongst ourselves, come up with some goals, and present them clearly for easier review
< morcos>
+100
< cfields>
jtimon: ^^
< wumpus>
#topic libconsensus
< cfields>
that was it :)
< NicolasDorier>
wumpus: we have not started the bikeshedding yet, expect longer fight about it soon :p
< luke-jr>
NicolasDorier: IMO just discuss it here
< luke-jr>
way too many channels already
< NicolasDorier>
the reason I prefer a separate channel is so we can keep history and review it more easily
< wumpus>
it's not like this channel is very busy\
< NicolasDorier>
things said here get lost quickly enough in the sea of discussion
< wumpus>
that always happens on IRC, if you want discussions with good history/memory then it may be better to use a different discussion mechanism, or keep summaries or such
< sipa>
i think having a separate place to 'form' a proposal makes sense
< luke-jr>
#bitcoin-dev is also fairly quiet
< sipa>
involving the whole world in a design rarely leads to something useful
< jtimon>
from what I talked with NicolasDorier in previous times, the current goal is to complete a verifyBlock without necessarily exposing it in libconsensus at first
< wumpus>
that's true, as long as you can get the people you want to pay attention to join it's fine
< wumpus>
sometimes it's good to not have too many people there
< wumpus>
but speaking for myself, I'm already in so many channels, I have a hard time following more
< morcos>
to be clear, the design will be presented to the bigger group for feedback and not set in stone
< btcdrak>
yeah ack on limiting channel proliferation, also it is more open in a logged channel
< morcos>
i don't plan on joining the libconsensus subgroup, but i think having a focused small group will make progress actually happen
< BlueMatt>
I'm with wumpus...too many channels these days (not that I've been paying enough attention to have much of a voice, but still)
< wumpus>
then again that's up to you, doesn't need to be a discussion topic here to decide onthat.
< jtimon>
I don't think creating a new channel or not is going to be important either way...
< cfields>
morcos: sure. doesn't matter to me where the discussion takes place, just hoping to organize the discussion/planning a bit.
< wumpus>
any other topics? 4 minutes to go
< sipa>
i'm in favor of having it in a separate channel because i prefer not to see all discussions around it before there is clean design :)
< NicolasDorier>
bikeshedding about the creation of the channel libconsensus announce the color of the future bikeshedding on the actual code ;)
< wumpus>
NicolasDorier: exactly
< wumpus>
let's stop that
< cfields>
heh
< sipa>
haha
< sipa>
3 minutes
< jtimon>
cfields: NicolasDorier if you can share the goals
< wumpus>
looks like we're done
< wumpus>
#endmeeting
< lightningbot>
Meeting ended Thu Aug 4 19:58:24 2016 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
< cfields>
jtimon: i think everyone has different goals in mind. the point of organizing is to settle on something and work towards it. i can catch you up on our discussions this weekend, though ofc you've spent more time on it than anyone
< jtimon>
cfields: well, a small summary of the "goals in common" or "easy to achieve" goals or something would be something
< NicolasDorier>
jtimon: I think we still have also to discover what the goals in common are. Which is why I think we can discuss it in the channel. I guess if the three of us agree (plus anybody in the channel), there is good chance the rest agree too
< jtimon>
NicolasDorier: I may not be so optimist, but fair enough, I mean, great. do you guys have time now to give me a little summary of your discussions so far now (in the other channel)?
< morcos>
It's related. The point is once you are enforcing BIP34 then BIP30 is unnecessary except for historical violations. We're changing the rule to have BIP34 be enforced as of a certain height, however, it kind of requires that the chain be the same otherwise we dont' know what the historical violations are or might be
< NicolasDorier>
yes, I will jtimon, but later I'll go back bed soon (5am here)
< jtimon>
yeah, I say so in my commit, it is unnecessary because we're not using ISM anymore
< jtimon>
oh, no worries, another time will be fine
< cfields>
jtimon: same, in the middle of several things atm. maybe some time tomorrow?
< jtimon>
sure, no problem
< NicolasDorier>
morcos: so basically we could remove BIP30 altogether as this is unlikely another chain manage to catch up the amount of work of the current one
< sipa>
isn't there some interaction between BIP30 and the no-overwrite optimization in coinscache?
< sipa>
which needs a BIP30 check during IBD
< NicolasDorier>
mmh I think I won't make any change about it after all... I'm not confident of this thing
< morcos>
NicolasDorier: yeah we need it on IBD
< morcos>
hmm this brings up a kind of good point
< morcos>
you can't remove the BIP34Hash unless you are going to checkpoint the chain at that point
< NicolasDorier>
ok, so I will not touch it
< jtimon>
you cannot remove it completely, but when you use bip34 you don't need to do it (this is current behaviour)
< morcos>
jtimon: no, when you use BIP34 on the existing BIP34 hash then you dont' need to do it
< morcos>
if you activated BIP34 at the same height on a different chain, it would be unclear whether yous till needed to do BIP30
< jtimon>
morcos: only one block?
< morcos>
depends on the status of historical violations on that chain
< morcos>
i can't remember off the top of my head, but if in an alternate chain you spent coinbase A before you generate coinbase A' then you activated BIP34, then you still need BIP30 to make sure the spend of A' doesn't conflict with the spend of A
< morcos>
but in the BIP34hash chain we know that didn't happen
< jtimon>
do you think that commit is incorrect after removing ISM?
< morcos>
jtimon: its incorrect in that it'll be broken code on a mainnet chain that doesn't contain BIP34hash
< morcos>
so who knows what happens if someone feeds you such a chain on startup
< jtimon>
I think it will only fail if we reorg below BIP34Height (227931)
< morcos>
oh wait
< morcos>
maybe because BIP30 is enforced for everythign except those two exceptions you're ok
< jtimon>
and what we have now, if it reorgs that long, without code for ISM...what? bip34 is going to be activated at that height no matter what
< jtimon>
well, I wouldn't call them exceptions
< jtimon>
pindexBIP34height will only be NULL if pindex->pprev->GetAncestor(227931) returns null
< jtimon>
what is doing now is that if you are above that height and the block you activated bip34 in is 227931, then don't enforce bip30 since bip34 is enforced
< jtimon>
in a reorg that deep, if it was activated with another block, then bip30 will be checked always regardless of bip34 activation (no optimization, never)
< morcos>
jtimon: i honestly don't think its worth doing. but if you really want to, i will take the time to think about it. there are a lot of issues, down to when you flush your cache. but please make an actual PR for me to review that other people also want to merge
< jtimon>
with my suggested change, it would always do the optimization after bip34 activation height, even after such a long reorg
< jtimon>
morcos: absolutely, no problem
< morcos>
jtimon: ok, i think i got the problem
< jtimon>
well, if you tell me now I won't open the PR :)
< morcos>
imagine that you reorg to height 90000 (or are just fed an alternate chain on startup)
< morcos>
ah shoot....
< morcos>
nm
< morcos>
wait
< morcos>
yes
< morcos>
thats it
< morcos>
so confusing
< jtimon>
let's do one thing at a time, yep, you reorg to 9000 or you fed an alternate chain?
< morcos>
either, same affect you start processing different blocks starting at 90000
< jtimon>
think the worse case: you reorg to block 1
< morcos>
thats after coinbase A was created but before its duplicate A' was created
< morcos>
now you spend coinbase A
< jtimon>
bip 30 is checked before bip34 always except the exceptions
< jtimon>
after bip34 activation, it is no longer necessary
< morcos>
then you create coinbase A' (allowed b/c it doesn't violate BIP30 , A is spent, and BIP34 is not active yet)
< morcos>
now you activate BIP34 at height 227931
< morcos>
then you can spend A' and create the same txid as your spend of A b/c you aren't enforcing BIP30 any more
< jtimon>
yes, from now on you can stop checking bip30 in new blocks
< morcos>
no you can't
< morcos>
spending A' would be allowed and would create a duplicate txid of the spend of A
< jtimon>
mhmm, whay they cannot today?
< jtimon>
why
< morcos>
because A' the coinbase was created before A was spent
< morcos>
so A' overwrote A already
< morcos>
but if in an alternate history you spent A to B first
< jtimon>
doesn't include the height make it very difficult to collide with coinbases before 227931 ?
< morcos>
then you have to worry about B' and B
< jtimon>
instead of A'
< jtimon>
think of all the coinbases before 227931
< jtimon>
no? you don't want the same as neither of those
< morcos>
you can try this on regtest.. enforce BIP30 only up to height 100 and BIP34 only starting at height 100. create some coinbases, spend them and then create duplicates before 100.
< jtimon>
that's with or without a reorg
< morcos>
then after 100 make them colide
< Chris_Stewart_5>
Is there a simple way to serialize a CKey? Doesn't seem to have the SERIALIZE_METHODS macro
< jtimon>
morcos: yeah, you could "attack regtest" that way...
< morcos>
well if you change the code, someone could feed you a messed up mainnet chain that way
< sipa>
Chris_Stewart_5: historically, it's converted from/to a CPrivKey for serialization
< jtimon>
I thought we were talking about mainnet
< morcos>
jtimon: yes so on mainnet, you'd have to have a REALLY deep reorg, below 227931
< Chris_Stewart_5>
sipa: Is there a simple utility function serialize CPrivKey? Can it just be passed to HexStr?
< morcos>
or if somoene just fed you an alternate low work chain then i'm not really sure what would happen, its possible that your utxo could get into a messed up state than you wouldn't be able to fix when you found out about the real chain
< jtimon>
and then a node without my change would always check bip30, even after bip34 activation, is that the desired behaviour?
< sipa>
Chris_Stewart_5: CKey also has begin() and end(), to it can be passed to HexStr directly
< sipa>
Chris_Stewart_5: and has a constructor that takes a byte array
< jtimon>
my node would not check bip30 after bip34, even after the reorg
< morcos>
jtimon: yes you can either always check BIP30 even after BIP34, or you can examine the chain you're on pre BIP34 and decide whether you can now skip BIP30 checks (which is what we decided to do with mainnet, but requires manual intervention)
< morcos>
are you opposed to leaving in the BIP34 hash
< morcos>
checking BIP30 is exteremely slow
< morcos>
that's why we put in the ability to skip it
< jtimon>
so, yes, theoretically always checking bip30 after bip34 has certain risk if there's that reorg...but aren't we screwed if that happens anyway?
< jtimon>
like, really screwed
< jtimon>
I don't know, maybe it's not worth it
< Chris_Stewart_5>
sipa: Thanks
< jtimon>
the advantages are code simplification and a slight optimization (really no need to call GetAncestor if you don't care about the hash), and keep the optimization in case of pre-bip34 reorg, which is arguably a disadvantage
< jtimon>
no, not opposed, removing it is just a suggestion
< jtimon>
now I'm afraid the right time to do that will be...never
< morcos>
jtimon: i think the issue is less a reorg than that someone might feed you an alternate chain on startup and get your utxo into a permanently messed up state b/c you don't know how to reorg back
< jtimon>
oh, I see
< morcos>
i'm not positive that would happen, but seems more likely than not, and also seems like it could be affected by future changes to the way coins work that we might not anticipate
< jtimon>
yeah, makes sense, thanks
< jtimon>
yeah, I wasn't properly thinking about an isolated node doing initial synchornization, that's what you meant by fed up with the wrong chain from the beginning, sorry
< jtimon>
nah, better to avoid that risk, definitely not worth it
< jtimon>
or when/if we expose consensus::Params in libconsensus
< jtimon>
anyway, that's another topic
< Chris_Stewart_5>
sipa: When I serialize CPriv/CPub I get something that is larger than 32 bytes?
< sipa>
Chris_Stewart_5: CPrivKey uses OpenSSL DER serialization... which is 300 bytes or so for a private key
< sipa>
CPubKeys are 33 or 65 bytes
< sipa>
CKey is internally 32 bytes + compression flag
< Chris_Stewart_5>
ahh ok, that makes more sense. So basically CKey is the interface exposed inside of core... except for serialization. Something that could be implemented in a pull?
< sipa>
well there isn't one single obvious serialization for CKey
< Chris_Stewart_5>
Seems like the 32 byte serialization would be the intuitive one.. at least to me. Am I missing something here?
< Chris_Stewart_5>
oh nvm i see what you are talking about now, since CKeys can be both priv and pubs right?
< sipa>
no
< sipa>
CKey is a private key
< sipa>
but you need the compression flag too
< sipa>
and there is no deserialize because we use secure memory for private keys where possible
< jtimon>
NicolasDorier: cool, but I believe the right time was in your PR, at least with the new ones if you didn't wanted to touch bip34
< NicolasDorier>
jtimon: I disagree, my PR was making a "big" consensus change so I wanted to keep it as simple as possible to review
< NicolasDorier>
I wanted to be it merged the quickier I could so I can continue my work on libconsensus
< jtimon>
although I won't oppose the refactor, of course, just worry that it will be less acceptable
< jtimon>
how would using an array instead of two variables have been more complicated?
< jtimon>
anyway, as said I won't oppose
< NicolasDorier>
jtimon: I don't think it wil lbe less acceptable, this buried deployment stuff will be a BIP. In the future other SF will end up using the same structure
< NicolasDorier>
anyway, I made two differents commit
< NicolasDorier>
we can take one and not the other
< jtimon>
NicolasDorier: yeah I guess the BIP is an opportunity to reorganize the struct
< jtimon>
anyway, I'm suffering from premature worry about controversy, let's not discuss about whether something will be acceptable or not when we both want it
< GitHub193>
[bitcoin] NicolasDorier opened pull request #8460: parametrize buried soft fork in regtest and refactor (master...buriedsf) https://github.com/bitcoin/bitcoin/pull/8460
< NicolasDorier>
jtimon: worrying about controversy create a meta controversy :D
< jtimon>
.MineBlocksOnDemand() is starting too look like IsRegtest()
< GitHub92>
[bitcoin] jlopp opened pull request #8461: document return value of networkhashps for getmininginfo RPC endpoint (master...rpcMiningHelp) https://github.com/bitcoin/bitcoin/pull/8461
< jtimon>
oh, it was RegTest() or Params().NetworkID() == CChainParams::REGTEST directly, never mind, it's been a while since 3824, thinking out loud
< jtimon>
NicolasDorier: in #8460, how do you modify a couple of them? -buriedsfparams=bip65:1351,bip66:1251 ?