< jonasschnelli>
phantomcircuit: Looks good. I really want to run some tests. I'll do that soon. But 8152 is definitively useful and will be merged soon.
< wumpus>
I'm still not entirely convinced that it won't reduce performance by causing more flushes to disk
< wumpus>
e.g. the reason for passing those CWalletDB in the first place (it wasn't always that way) was to prevent extraneous flushes, as sipa says in that pull
< wumpus>
but if anyone can convince me that that problem has been handled in another way I'm ok with merging it
< wumpus>
(or even better, benchmarks)
< wumpus>
jonasschnelli: re: 8407, I think the second commit makes the code unncesarily complex; I think we can already handle all upgrade scenarios without it
< jonasschnelli>
wumpus: IMO 8152 is only a refactoring to avoid having CWalletDB access in rpc code.
< jonasschnelli>
wumpus: re 8407, I agree. I will remove the second commit asap
< wumpus>
yes avoiding CWalletDB access in the RPC code is great; but that could also have been accomplished by adding a wrapper method in CWallet
< wumpus>
that passes through the wtx to AddToWallet and opens the walletdb
< wumpus>
what I'm worried about is the internal uses of AddToWallet that do pass their own walletdb
< wumpus>
eh - do those exist at all?
< jonasschnelli>
wumpus: I have checked the internal AddToWallet uses. And it seems like they all open CWalletDB shortly before resulting in the ~same behavior.
< jonasschnelli>
I think the change form (external calls) pwalletMain->AddToWallet(wtx, false, &walletdb); to pwalletMain->AddToWallet(wtx); is good.
< jonasschnelli>
It hides CWalletDB behind CWallet
< wumpus>
hmm I agree
< jonasschnelli>
(for callers using AddToWallet)
< wumpus>
LOL @ line 910
< jonasschnelli>
But as I said. I think its a refactoring PR
< jonasschnelli>
wumpus: you mean the return false at L910
< wumpus>
of wallet.cpp - that line could be deleted, no one is using that walletdb :-)
< jonasschnelli>
Its a required 0.13 backport and relatively risk free
< wumpus>
jonasschnelli: yes I was planning to test it, will do so
< jonasschnelli>
Thanks
< wumpus>
jonasschnelli: a small nit: cVjNjjiDXCsTZKVP3VzjH3PPdNdeS7t51RXJ6rt99GgnzaTFtrxz 2016-07-27T13:00:33Z label= # addr=mfZRKaG9dfW5xEqUWiJFFFohe8XXoW297N hdkeypath: m/0'/0'/0'
< wumpus>
let's format it hdkeypath= instead of hdkeypath:
< wumpus>
that's more consistent
< jonasschnelli>
ah. right. Let me change that directly
< wumpus>
thanks
< wumpus>
apart from that it works, tested with a non-HD and a HD wallet
< jonasschnelli>
wumpus: what do you think about changing "oldhdmaster" to "inactivehdmaster"?
< jonasschnelli>
oldhdmaster sound not ideal
< wumpus>
yes seems more on-point
< jonasschnelli>
okay. Will change that as well
< wumpus>
it not so much matters whether old or new, just that it's not used at the moment
< wumpus>
or maybe a more heretical idea: number the master keys, and store what master key was used with the key metadata
< wumpus>
maybe for 0.14
< jonasschnelli>
wumpus: yes. Though about that. But would add nother "long" string to each line
< jonasschnelli>
or we could add an index to each master key (for simplification)
< wumpus>
yes indeed my idea was an index/handle, not the full master key id
< wumpus>
that would indeed be ugly, and would take up significant extra space in the metadata
< jonasschnelli>
Yes. This would make sense for 0.14 I guess.
< jonasschnelli>
https://github.com/bitcoin/bitcoin/pull/8206 is featureish and I think we only backport it to 0.13 because of possible complains because of lack of exporting the xpriv
< wumpus>
yes I agree we don't want to do this for 0.13
< wumpus>
it was just a wild idea
< wumpus>
for 8206 let's just fix the output format and merge it
< jonasschnelli>
wumpus: I think 8206 is ready now
< GitHub42>
bitcoin/0.13 18b8ee1 Jonas Schnelli: [Wallet] add HD xpriv to dumpwallet...
< wumpus>
8389 needs rebase
< jonasschnelli>
rebased
< wumpus>
jonasschnelli: did you see my remark here: https://github.com/bitcoin/bitcoin/pull/8407/files#r72257114 ... I'm not sure what settings.value(strSettingsVersionKey) returns in the case !settings.contains(strSettingsVersionKey), but if it's garbage or raises an exception this may be problematic
< jonasschnelli>
wumpus: yes saw it. But I thought the second expression in the if gets not executed if the first one (.contains()) fails. It's an OR.
< wumpus>
yes that is true, but look inside the {} there is another settings.value(strSettingsVersionKey)
< wumpus>
if (settings.value(strSettingsVersionKey) < 130000 && ...
< jonasschnelli>
Ah. Right!
< jonasschnelli>
That needs to be fixed. Will do soon (afk/phone typing).
< GitHub191>
bitcoin/0.13 0179a39 Wladimir J. van der Laan: qt: periodic translations update
< goatpig>
hi
< goatpig>
you don't need the mask and flag when creating a tx with only segwit outputs, do you?
< goatpig>
as in, no segwit inputs redeeming, only creating segwit outputs
< goatpig>
non nested
< luke-jr>
goatpig: that is my understanding, correct
< luke-jr>
furthermore, I think you can even include such outputs in non-segwit blocks. albeit, they would be vulnerable to easy theft until segwit activates.
< luke-jr>
(but it's possible to mine non-segwit blocks even after segwit activates, so not entirely useless)
< goatpig>
in this case they would just behave as anyone can spend wouldn't they? at least until miners start enforcing segwit rules
< luke-jr>
right
< goatpig>
so the proper guideline would be to add mask and flag when creating non nested segwit outputs, regardless of inputs?
< luke-jr>
even after segwit activates, libblkmaker will produce non-segwit blocks if there are no witness-as-input transactions being mined
< gmaxwell>
Well they're non-standard currently.
< luke-jr>
goatpig: I don't understand your question there
< goatpig>
im thinking for after segwit is activated, I don't intent to allow ppl to create SW tx with Armory until a few weeks after that happens on the mainnet
< luke-jr>
IIRC, it's invalid to have dummy/flag if there's no witness data for inputs
< goatpig>
luke-jr: oic
< luke-jr>
(I could be wrong on that, but IIRC)
< goatpig>
wouldn't that mean that you can't mix regular and sw outputs in a same tx?
< luke-jr>
goatpig: no, you can have dummy/flag so long as at least one input needs witness data
< goatpig>
ok
< goatpig>
aight that sums it for me
< goatpig>
thanks for the help
< jtimon>
mhmm, there's a bitcoinconsensus_SCRIPT_FLAGS_VERIFY_WITNESS but not a bitcoinconsensus_SCRIPT_VERIFY_CHECKSEQUENCEVERIFY in script/bitcoinconsensus.h...
< sipa>
jtimon: good point, we should fix that
< jtimon>
I'll PR it, just wanted to confirm first
< jtimon>
sipa: thanks
< jtimon>
actually found out while adapting my consensus branch to ScriptFlagsFromConsensus()as discussed (WIP)
< GitHub48>
[bitcoin] jtimon opened pull request #8412: libconsensus: Expose a flag for BIP112 (master...0.13-consensus-bip112-flag) https://github.com/bitcoin/bitcoin/pull/8412
< jtimon>
does this little thing break rc1?
< GitHub132>
[bitcoin] jtimon opened pull request #8413: Trivial: pass Consensus::Params& instead of CChainParams& in ContextualCheckBlock (master...0.13-consensus-last-params) https://github.com/bitcoin/bitcoin/pull/8413