< cfields>
jtimon: i think you want something like: sed -i 's/BOOST_FOREACH(\(.*\),\(.*\))/for(\1 :\2)/' net_processing.cpp
< cfields>
?
< jtimon>
oh, yeah, the dot, thank you very much
< cfields>
np
< cfields>
jtimon: you'll need to filter some things out of that. iirc pairs are handled differently, at least.
< jtimon>
I shouldn't even need a pair I think, now I'm trying sed -i "s/BOOST_FOREACH(\(.*\), /for (${\1} :" src/net_processing.cpp
< jtimon>
bash: s/BOOST_FOREACH(\(.*\), /for (${\1} :: bad substitution
< jtimon>
it feels like it's something embarrasingly obvious
< jtimon>
ok, "git checkout -- ." was the first thing I was missing before trying again with something different
< jtimon>
alright, I think 'git checkout -- . ; sed -i 's/BOOST_FOREACH(\(.*\),/for (\1 :/' ./src/qt/*.cpp ./src/wallet/*.cpp' is enough to test your PR on travis
< cfields>
jtimon: does that actually build?
< jtimon>
cfields: sed does what I expect, and only with ./src/qt/*.cpp , it passes unittests
< cfields>
mm, neat
< jtimon>
now it's time to make it "fail" on purpose and open a PR, then add a fixup commit to be squashed once your pr is merged
< jtimon>
neat indeed, I expect this to be revolution in refactors, thanks again
< cfields>
:) happy to help
< cfields>
I've had this one (the cnode change) done on a ton of branches, but never felt like dealing with the process of pushing it through. So yea, I can see how it could be helpful for lots of similar changes.
< jtimon>
at the very least, it revolutionized the way I think about refactors, maybe it was obvious to use sed for rebase and review for everyone else but certainly not for me
< jtimon>
yeah, not only painful simple changes will stop to be painful
< jtimon>
which is the fisrt use case
< jtimon>
but also some painful changes that authors don't even open as PR because they're too disruptive will be open now
< cfields>
awesome
< jtimon>
and more importantly, reviewed too
< cfields>
jtimon: you might look at pairing it with "git rebase -i --exec <script>" too :)
< cfields>
for maintaining
< jtimon>
or maybe I'm over-excited about this, it is good enough to know I am not stupid for not thinking about this by myself beforehand
< jtimon>
-exec was failing locally for some reason
< cfields>
heh
< cfields>
headed off, nnite
< bitcoin-git>
[bitcoin] jtimon opened pull request #10193: scripted-diff: sed -i 's/BOOST_FOREACH(\(.*\),/for (\1 :/' ./src/qt/*.cpp (master...b14-10189-scripted-qt-foreach) https://github.com/bitcoin/bitcoin/pull/10193
< cfields>
jtimon: you didn't format the commit message in a way that it will be picked up
< bitcoin-git>
[bitcoin] bulldozer00 opened pull request #10194: Remove unecessary friend keyword from the class definition (master...patch-1) https://github.com/bitcoin/bitcoin/pull/10194
< bitcoin-git>
[bitcoin] jnewbery opened pull request #10198: [tests] Remove is_network_split from functional test framework (master...remove_is_network_split) https://github.com/bitcoin/bitcoin/pull/10198
< bitcoin-git>
[bitcoin] jnewbery closed pull request #10198: [tests] Remove is_network_split from functional test framework (master...remove_is_network_split) https://github.com/bitcoin/bitcoin/pull/10198
< arubi>
is there a way to get bitcoind to not complain about any non-standardness? I'd still like for it to error on operations with invalid transactions, but for example I'd like it to not care about DER strictness, or to ignore the P2SH requirement for a valid redeemscript (provided some preimage to p2sh, it should pass, no matter if the preimage parses as a script at all). "SCRIPT_VERIFY_NONE = 0;" in script/interpreter.h (I'm on some
< arubi>
0.13.99) looks promising, but I'm not sure what to set it to. would love a hint on what I should look for
< arubi>
this is for testnet \ regtest use by the way
< arubi>
maybe I should just set all verify flags to 0 :)
< Chris_Stewart_5>
but I'm not sure if you can do this from the command line :/
< arubi>
oh no command line, these flags are not enough though
< arubi>
it's letting it advance, I can send non standard transactions with some of these flags disabled, but getblocktemplate fails
< arubi>
I don't wanna say that it's related, I already mauled this specific code to death
< Chris_Stewart_5>
hmmm, perhaps you need to tinker with relay policy? Not sure where that is set in the codebase though
< arubi>
other nodes drop the transaction
< arubi>
actually I think they're banning me
< arubi>
I'll be able to mine it myself eventually on testnet, when the diff drops, I just need getblocktemplate to work :) setting all verify flags to 0 now
< Chris_Stewart_5>
setting all the flags to zero worked?
< arubi>
yep
< Chris_Stewart_5>
you couldn't just unset STRICTDER?
< arubi>
well I noticed script_verify_p2sh is used even with this tx's bare p2pk
< arubi>
I did unset strictder to even be able to relay it... I think checking block validity is even more strict than that
< arubi>
if you saw the tx in #-dev, it's also a hybrid pubkey, so maybe more relaxed flags are needed
< Chris_Stewart_5>
arubi: Is p2pk scripts considered non standard?
< arubi>
if it's a hybrid pubkey, probably
< arubi>
also the signature itself is weird. uses 0x01000000 as the sighash when the sig is passed as input
< arubi>
so it's actually passed with a 4 byte value instead of just 0x01.. I think it should still be valid?
< instagibbs>
morcos, for #10199 did you look at how it handles chain limits? There was some speculation during last spam attack that the chain limits were causing high-fee transactions to "fail" to get into blocks, spiking fees randomly
< Chris_Stewart_5>
to see if the hash type is valid
< arubi>
being defined != being valid :)
< arubi>
the famous amount overflow transaction used 0xA8 as sighash iirc, weird, but it resolves to ALL
< arubi>
interpreter.cpp has the rules for setting up the sighash, it does a bunch of bitwise and's with 1F and flags it knows. really I think 32 bits can be used. sighashv2 uses 32 bits
< instagibbs>
morcos, ok good to know. One less idea why someone was doing that then.
< arubi>
er, it uses 16 bits.
< arubi>
was just informed that I may be wrong re. 4 bytes sighash value :)
< arubi>
bip66 might be causing testnet failures where it works on regtest. will stop spamming :)
< Chris_Stewart_5>
Yeah when checking the signature you have to prepend? the extra bytes?
< bitcoin-git>
[bitcoin] sdaftuar opened pull request #10200: Mining: Skip recent transactions if fee difference is small (master...2017-04-dont-mine-recent-tx) https://github.com/bitcoin/bitcoin/pull/10200
< gmaxwell>
sdaftuar: wow, that is much more complicated than I expected. What I had just expected it to do is to simply skip very new transactions unless they paid a high feerate, just like we skip transactions that there aren't room in the block for.
< gmaxwell>
sdaftuar: is there a big fee income difference from doing a simple thing like that?
< BlueMatt>
cfields: wait, we really want random bash snippets in git history run by a script in #10189? I'm unsure about the wisdom of that?
< gribble>
https://github.com/bitcoin/bitcoin/issues/10189 | devtools/net: add a verifier for scriptable changes. Use it to make CNode::id private. by theuni · Pull Request #10189 · bitcoin/bitcoin · GitHub
< BlueMatt>
i mean its super nice to have, but also...running bash scripts out of git messages :/
< cfields>
BlueMatt: arguably if a script is used to transform a large chunk of code, it should be saved with the commit for future reference. Why not use it too?
< BlueMatt>
yea......
< BlueMatt>
cfields: do merge tools show commit messages very clearly for sign-off?
< BlueMatt>
would need to if they dont for this, i suppose
< cfields>
BlueMatt: "git notes" is what you're asking about, i think :)
< BlueMatt>
hmm?
< cfields>
BlueMatt: i suppose I don't understand your question
< BlueMatt>
i would expect them to clearly query the merger to read each commit's commitmsg
< cfields>
unsure. Wouldn't change anything there, though. the merge script could just run the verifier first.
< BlueMatt>
cfields: oh dear god lets not default to running random peoples' commitmsg scripts on wumpus' computer
< BlueMatt>
cfields: my point is that many people dont read commitmsgs in enough detail and might miss such scripts esp if they're replaced last-minute in a "title-only rebase"
< cfields>
BlueMatt: travis runs the script and fails if it doesn't transform 1:1. It can be done locally as well....
< BlueMatt>
cfields: insmod cool_thing.ko && sed s/BOOST_FOREACH/for/
< BlueMatt>
it'll transform 1:1 still?
< BlueMatt>
this is not a sufficient check
< BlueMatt>
automating on travis whatever
< BlueMatt>
putting yet more scripts in a place that people might not see it is bad...I'm only suggesting we make it more apparent at least to people pressing the merge button
< BlueMatt>
as many of us primarily just look at the diff itself
< * BlueMatt>
ponders if you can get a ^H somewhere....insmod kool_things.ko &&^H^H^H^H^H^H^H^Hsed s/BOOST_FOREACH/for/
< BlueMatt>
probably not
< cfields>
BlueMatt: the person pressing the merge button on bac5c9cf643e9333479ac667426d0b70f8f3aa7f should be running that sed script to be sure all occurances have been caught. Again, this should only be automatiting cases where mass transforms need to be checked anyway.
< BlueMatt>
cfields: yes, but the person pressing the merge button should also be reading the script before it gets automatically run on their system
< BlueMatt>
I think we're talking past each other somehow
< cfields>
so yea, maybe not make it part of the merge script, but i don't see how having c-i do it could be a bad thing
< BlueMatt>
I didnt say having c-i do it is bad?
< BlueMatt>
i agree, travis should do it?
< BlueMatt>
my only point was that the person pressing the merge button MUST be forced to read commit messages now
< BlueMatt>
<BlueMatt> I think we're talking past each other somehow
< cfields>
ok, fair enough. I thought you point was that the person hitting the button should be verifying as well if it's going to live in the commit message..
< BlueMatt>
oh i mean maybe, i dont care much either way there, travis should run it so thats good
< BlueMatt>
my only point is something should be done because many of us dont read commit messages as part of review (much)
< cfields>
BlueMatt: jtimon suggested a prefix: "scripted-diff: commit msg here". I suppose for those, the merge-tool could interrupt and present the full message, if it doesn't already
< BlueMatt>
that may help, i suppose
< BlueMatt>
long prefix sucks though
< BlueMatt>
scripted:
< cfields>
well it could also just detect the script begin/end
< BlueMatt>
but, yea, putting it in the commit title itself is probably good
< cfields>
jtimon: btw, I think you're looking for: sed -i '/#include <boost\/foreach.hpp>/d' file
< cfields>
:)
< jtimon>
btw, how can I do sed -i ':a;N;$!ba;s/#include <boost\/foreach.hpp>\n//' ./src/*.cpp but excluding some specific files ?
< jtimon>
yeah, for qt and wallet I have it done, but some places use BOOST_REVERSE_FOREACH so they need to maintain the include for now
< jtimon>
that's why I need to exclude specific files
< sdaftuar>
gmaxwell: yeah it is certainly possible i overlooked something simple
< sdaftuar>
gmaxwell: but with what you describe, i don't see how you could bound the income difference?
< sdaftuar>
without making assumptions about the fee distribution
< cfields>
jtimon: how about just changing those to rbegin/rend iterators in a prior commit?
< jtimon>
yeah, that would be another option, at first I was thinking of only removing it from qt and wallet for now, but if I can completely remove them using this, it doesn't look as disruptive as I expected
< sdaftuar>
gmaxwell: one way to implement what you describe might be, fill up the first X% of the block, and if no recent transactions were chosen, then exclude recent transactions from the remainder of the block
< sdaftuar>
but if fee distributions were close to flat, then you might give up a lot of income unless X is large
< jtimon>
I mean, if people are ok with doing it all at once, I would prefer it
< BlueMatt>
jtimon: please. kill boost
< jtimon>
BlueMatt: alright, that feeback is useful
< BlueMatt>
:)
< sdaftuar>
gmaxwell: and conversely, i think you could also have the problem of including recent transactions too frequently -- a small high feerate transactions that was recently received should almost certainly not be included
< sdaftuar>
because block reward is still high enough that it dominates
< bincap>
is it possible to write a script that executes or not depending on version bits of current block? (e.g. a payment locked by activation of segwit)?
< bincap>
(or of other block)
< sipa>
no, transactions cannot observe what block they are in
< sipa>
otherwise you couldn't validate them before being confirmed
< bincap>
sipa: in previous block(s) then
< sipa>
no, transactions cannot observe what block they are in
< gmaxwell>
sdaftuar: income difference is bound by amount_you_will_skip times transactions.
< bincap>
sipa: could they instead observe e.g. version bits of block defined in script by it's height?
< bincap>
that could allow to set up some incentives for users promising to support given BIP to actually do that
< gmaxwell>
sdaftuar: I agree that what you propose is more 'correct', but it is non-trivially more complex. I'm doubtful that it increases income a meaningful amount, we've also not seen miners working especially hard in increasing their work update speed.. which causes similar losses.
< sipa>
transactions exist before they're in a block
< sipa>
their validity is independent of what chain they are included in
< sipa>
so no
< bincap>
I see. I thought of something like NLockTime
< bincap>
can you otherwise create now a transaction, that will be very expensive to spend without segwit, but easy with segwit? (for example some setup of creating dust transactions and collecting them in segwit)
< gmaxwell>
bincap: such things would create terrible incentives to lie about your rule support. These kinds of proposals have been rehashed many times before...
< berndj>
gmaxwell, i think i started this line of thinking in #bitcoin, my version was only slightly different, where the coins become redeemable by anyone (i.e. probably a miner) if SW should fail to be active
< berndj>
so blame me for that :)
< sdaftuar>
gmaxwell: i don't think the PR is all that complex, it may appear to be because i broke things out into lots of small commits (hopefully, to help review). but admittedly i introduced some complexity in order to shave a few milliseconds off the runtime
< gmaxwell>
sdaftuar: to confess, I didn't review it yet but you made it _sound_ complex.
< sdaftuar>
gmaxwell: conceptually, i could have kept things very simple if i just ran CNB twice, once allowing recent transacitons in, and once without them
< gmaxwell>
yes, but that would have pretty poor performance.
< sdaftuar>
not terribly poor, if you avoided the TBV call
< sdaftuar>
er, redundant TBV call
< sdaftuar>
basically you would just call addPackageTxs twice... average runtime of 7-8ms
< sdaftuar>
so that times two, versus the optimization i did, which gets down to about 10ms instead
< sdaftuar>
i do want to save further optimization for a future PR. but i could back out the optimization in this one, if it would aid review
< gmaxwell>
Do you have a benchmark for this, that lets you measure how often it picks each option and how much fees it loses on consistent traffic?
< gmaxwell>
e.g. that you could also try a braindead simple patch that just skips txn you've had less than X seconds, unless they have astronomic fees?
< sdaftuar>
yeah i have simulated this a bunch. i think with 10seconds of recency and 1% threshold, it almost never will include recent transactions.
< sdaftuar>
my most recent run was with 30 seconds/0.5% threshold, and it still very rarely included recent transactions... maybe a handful of times out of more than 1000 samples
< gmaxwell>
I made measurements of cross node mempool consistency a while back, but cdecker tells me that propagation has become much slower (which was intentional) so those numbers probably should be redone.
< sdaftuar>
so the toggles in that model would be X (number of seconds) and fee (feerate?) threshold?