< sipa>
sdaftuar: is there a good reason to support that?
< BlueMatt>
i dont think its "permitted"
< BlueMatt>
there should be no way to call it except permiscuousmempoolflags
< sipa>
i mean: we should have an assert for it
< BlueMatt>
which is a testing-only thing
< sipa>
just as there is an asserts that prevents CLEANSTACK without P2SH/WITNESS
< BlueMatt>
meh? I mean its testing-only, if it doesnt crash why bother making it?
< BlueMatt>
just complicates testing of it
< BlueMatt>
it being script generally
< sipa>
i like that every script execution flag is a softfork wrt any other combinations of script execution flags
< sipa>
i think upgradable nops is the only exception
< PRab>
nm, I got it. I had to add --upgrade to the end of the existing gbuild command.
< bitcoin-git>
[bitcoin] practicalswift opened pull request #10686: Avoid usage of uninitialized values in function call arguments (master...uninitialized-arguments) https://github.com/bitcoin/bitcoin/pull/10686
< bitcoin-git>
[bitcoin] laanwj closed pull request #10649: Make sure we only mine via the first wallet (master...2017/06/wallet_generate) https://github.com/bitcoin/bitcoin/pull/10649
< wumpus>
PRab: yes, upgrades are not persistent, it happens every gbuild on the copied image
< wumpus>
there's no documented way to upgrade the base image - I tried it last time but ended up with 3 versions of gcc installed and a wrong gitian output :)
< morcos>
As an RPC argument, fundrawtransaction uses optIntoRbf, bumpfee uses replaceable, my recent PR used opt_in_rbf for sendtoaddress and sendmany. Should I instead use replaceable? (I felt that was maybe too general a word) Or should I change the preexisting argument names?
< wumpus>
morcos: from the developer notes, RPC interface guidelines: Argument naming: use snake case `fee_delta` (and not, e.g. camel case `feeDelta`)
< wumpus>
so opt_in_rbf or replaceable are ok
< wumpus>
it's unfortunate that there is such a zoo of different argument names for the same
< wumpus>
for API changes introduced since 0.14 there's still the possibility of normalizing them
< morcos>
wumpus: oh looks like optIntoRbf was just merged
< morcos>
so what do you think, make them all replaceable? and not worry that it sounds a bit too generic
< morcos>
seems fine to me, the help can always clarify it means BIP-125 replaceable
< morcos>
and we don't have any other notion for now
< wumpus>
sounds good to me
< morcos>
ok will do
< wumpus>
general is good from an API viewpoint I guess - the user might not care how the replacement works, just that the transaction can be replaced
< wumpus>
(on the other hand, if different kinds of replacement would be supported, they might want to choose a certain one based on their specific properties... but meh, could always add an additional field for replacement type)
< morcos>
luke-jr: you ok with changing optIntoRbf and optintorbf that you added in #9672 to replaceable? should i also change rbfoptin from bitcoin-tx (same PR) ?
< bitcoin-git>
bitcoin/master aa95947 practicalswift: Use the override specifier (C++11) where we expect to be overriding the virtual function of a base class
< bitcoin-git>
bitcoin/master 9a941a1 Wladimir J. van der Laan: Merge #10631: Use the override specifier (C++11) where we expect to be overriding the virtual function of a base class...
< bitcoin-git>
[bitcoin] laanwj closed pull request #10631: Use the override specifier (C++11) where we expect to be overriding the virtual function of a base class (master...overrides-ii) https://github.com/bitcoin/bitcoin/pull/10631
< sdaftuar>
sipa: i think i agree with you that it'd be nice to assert if DISCOURAGE_UPGRADABLE_NOPS is given, but CLTV/CSV are not
< sdaftuar>
also i assume (someday) DISCOURAGE_UPGRADABLE_WITNESS_PROGRAM could have the same issue
< sdaftuar>
BlueMatt: i think testing is more complicated, not less, by allowing that combination of script flags
< BlueMatt>
sdaftuar: fair
< sipa>
sdaftuar: so, say we adopt a new OP_NOP4 at some point, and a softfork is proposed to deal with it
< sipa>
we'd make it subject to DISCOURAGE_UPGRADABLE_NOPS only when the new flag is not set
< sipa>
with an assert that requires upgradable_nops to also have the new SF active, it means you can't enforce a policy of rejecting such transactions in the mempool before the SF happens
< sipa>
i wonder if that's how we've always done softforks
< sdaftuar>
i don't think i follow --
< sdaftuar>
presumably the software that introduces the new OP_NOP4 thing will immediately enforce it in the mempool?
< sipa>
sure
< sdaftuar>
upgradable_nops doesn't require the new SF is active, it just requires that your new policy is active
< sdaftuar>
since upgradable_nops is only ever policy
< sipa>
sure
< sdaftuar>
oh!
< sdaftuar>
now i understand
< sipa>
but i mean: it seems strange to allow (valid) OP_NOP4 transactions in the mempool before the SF activates
< sipa>
they'd be mined too
< sdaftuar>
right, we had this issue with CSV i think
< sdaftuar>
where we added a special rule on version 2 transactions being rejected pre-activation
< sdaftuar>
for CLTV, i think we just allowed valid CLTV transactions in the mempool pre-activation
< sdaftuar>
well, i think what we did for CSV was the right thing.
< sipa>
for CSV it worked by keeping v2 transactions nonstandard until needed, i think
< sdaftuar>
yep
< sipa>
that won't work for every SF
< sdaftuar>
well the OP_NOP style of SF is probably not long for this world, no?
< sdaftuar>
i think the question is how do we envision it working in segwit
< sipa>
same applies to upgradable_witness_program
< sdaftuar>
so upgradable_witness_program definitely shouldn't be in the mempool pre-activation, i think.
< sipa>
right
< sipa>
but i think that requires 3 script flags
< sipa>
1) discourage_upgradable_witness_program (which does not apply anymore to v1 witness programs)
< sipa>
2) discourage v1 witness programs
< sipa>
3) enforce v1 witness program rules
< sipa>
chain uses none before activation, (3) after
< sipa>
mempool uses (1)+(2) before activation, (2)+(3) after
< sipa>
eh, (1)+(3) after
< sdaftuar>
i think that makes sense, conceptually. it would be nice if (2) didn't need to waste a script flag bit, though.
< sipa>
(2) could be twmporary i guess
< sipa>
as logic for the pre-activation mempool case isn't needed long term
< sdaftuar>
agreed
< sipa>
though it may be of longer term use in tests
< bitcoin-git>
[bitcoin] wraith7 opened pull request #10691: Trivial: Properly comment about shutdown process in init.cpp file. (master...master) https://github.com/bitcoin/bitcoin/pull/10691
< bitcoin-git>
[bitcoin] TheBlueMatt opened pull request #10692: Make mapBlockIndex and chainActive and all CBlockIndex*es const outside of validation/CChainState (master...2017-04-const-mapblockindex) https://github.com/bitcoin/bitcoin/pull/10692
< bitcoin-git>
[bitcoin] practicalswift opened pull request #10694: Remove redundant code in MutateTxSign(CMutableTransaction&, const std::string&) (master...remove-redundant-code-in-MutateTxSign) https://github.com/bitcoin/bitcoin/pull/10694
< morcos>
I'm trying to clean up some of the wallet fee logic around minimums and maximums and set fees vs estimates.
< morcos>
When should we obey the global maxtxfee?
< morcos>
Right now we do not obey that for fundrawtransaction, but the changes I was lookign at making would refactor so that fundrawtransaction does obey that
< BlueMatt>
sounds good
< BlueMatt>
consistency above all, imo
< BlueMatt>
actual decision isnt critical, consistency is
< morcos>
But I could see how fundrawtransction is a different beast that maybe you want to be able to ignore that setting for.
< morcos>
similar question for obeying minimums
< instagibbs>
Consistency is my expectation as a user of that api
< instagibbs>
(controls to override are totally fine ofc)
< morcos>
luke-jr: the main issue is the only argument name already in release is part of bumpfee and it's already called replaceable
< morcos>
so while i agree that opt_in_rbf or similar would be even better
< morcos>
i'm not sure its worth breaking existing API or having multiple names that mean the same thing
< morcos>
and i think perhaps we should just go with replaceable for now?
< morcos>
but i could be convinced otherwise
< luke-jr>
morcos: they're all in release (Knots)
< luke-jr>
but as long as the current name is retained as an alias, it doesn't really break the API
< BlueMatt>
bitcoin core does not support Knots releases, so I dont think you can argue we're tied to Knots' API - Knots can apply patches if it likes
< luke-jr>
Bitcoin Core doesn't NOT support Knots releases either. Knots APIs are not necessarily guaranteed, but we should try to maintain compatibility when reasonable.
< cfields>
BlueMatt: I'm looking at #10652 now. Can you give a bit of context for "Turn mapBlocksInFlight into a multimap" ?
< bitcoin-git>
[bitcoin] practicalswift opened pull request #10696: Remove redundant nullptr checks before deallocation (master...delete-nullptr) https://github.com/bitcoin/bitcoin/pull/10696
< cfields>
if so, please consider ^^ a request for a more thorough commit message :)
< bitcoin-git>
[bitcoin] TheBlueMatt opened pull request #10697: Do not hold cs_vNodes when making ForEachNode Callbacks (master...2017-06-cnodestateaccessors-5) https://github.com/bitcoin/bitcoin/pull/10697
< BlueMatt>
heh, naa, I was just splitting the pr
< BlueMatt>
cfields: those commits are rebased from #9447
< BlueMatt>
cfields: lemme take a crack at a more expansive commit message since they now lack the context of actually adding simultaneous block downloads
< cfields>
BlueMatt: thanks. the PR title answers my question, though. I just wasn't sure what exactly it was setting the stage for.
< BlueMatt>
yea, its setting the stage for two things - parallel block downloads but also having per-CNodeState locks
< BlueMatt>
the per-CNodeState locks need that change so that you dont hold two at the same time (which would be a lockorder violation)
< BlueMatt>
so you need to MarkBlockAsReceived in the background (ie via CValidationInterface callbacks) and not access the mapBlocksInFlight entries of other peers when you go to download a block
< bitcoin-git>
[bitcoin] TheBlueMatt opened pull request #10698: Be consistent in calling transactions "replaceable" for Opt-In RBF (master...2017-06-replaceable-rpc-args) https://github.com/bitcoin/bitcoin/pull/10698
< cfields>
BlueMatt: you beat me to 10697 by a day. would it irritate you if i PR'd an alternative? switches to shared/weak ptrs and drops the manual refcounting.
< BlueMatt>
cfields: no, please do!
< BlueMatt>
cfields: my only deadline is...uhh...long after 0.15 :p
< cfields>
BlueMatt: ok. I've been trying to work out how to do it safely (stupid threading), but I think I worked out something dead-simple
< BlueMatt>
cool, sounds good
< cfields>
BlueMatt: well, if I don't get it pushed today/tomorrow, I'll go ahead and ACK yours.
< BlueMatt>
our deadlock detection assumes it is the case, and will otherwise give useless results
< BlueMatt>
because we appear to currently never violate that requirement, better to enforce now instead of risking it breaking in the future and ending up with no working deadlock detection (without running ThreadSanitizer
< BlueMatt>
)
< cfields>
oh, so atm if we have 2 locked, and we invert the unlocks, it just sees that they're both unlocked and doesn't complain?
< BlueMatt>
if we lock A, then lock B, then unlock A, then re-lock A, that may create a deadlock, but will not be detected
< BlueMatt>
cause when we unlocked A, DEBUG_LOCKORDER just assumed that we were unlocking B cause it was the most recent lock
< cfields>
ah, ok
< BlueMatt>
if we ever need it we can tweak DEBUG_LOCKORDER, but we dont now, so easier to just assert correctness in our debug tool and move on
< sipa>
we can easily add a test to verify that an unlock is in fact the last-locked mutex
< BlueMatt>
sipa: yup, thats what I did, just added an assert...its only in debug code anyway
< gmaxwell>
::sigh:: master isn't compiling for me.
< gmaxwell>
qt/libbitcoinqt.a(qt_libbitcoinqt_a-paymentserver.o): In function `payments::Payment::set_merchant_data(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)':
< gmaxwell>
/home/gmaxwell/bq/src/qt/paymentrequest.pb.h:1600: undefined reference to `google::protobuf::internal::kEmptyString[abi:cxx11]'
< bitcoin-git>
[bitcoin] sipa opened pull request #10699: Make all script validation flags backward compatible (master...20170628_softflags) https://github.com/bitcoin/bitcoin/pull/10699