< dongcarl>
What are people's thoughts on having a static analyzer run by default on PRs?
< sipa>
dongcarl: the question is how it will report, and what the policy would be around it
< sipa>
for things that are reasonably false positives, you can't force contributors to avoid them
< dongcarl>
sipa: I think that the workflow should be this: CI shouldn't fail if the static analyzer notices problems, instead, once the static analyzer is done, a bot (like DrahtBot) posts a link to the report summary on the PR. Reviewers can take a look at the report, or not, completely supplementary info.
< dongcarl>
clang's static analyzer actually generates html for you, pretty convenient
< sipa>
dongcarl: also spamming PRs with things that aren't actionable is very annoing
< dongcarl>
Hmmmm... Okay. I think what's been happening currently is that every quarter or so kallewoof posts the static analyzer output as an Issue and people dig thru it... Perhaps that's less noisy?
< sipa>
dongcarl: i think it would be useful to have some separate page where you can see an overview of various analysis tools run on the PRs
< sipa>
but that's a lot more work obviously
< dongcarl>
sipa: Hmmm... I remember seeing some site like this but for benchmarking? Can't remember who's it was...
< sipa>
jamesob's ?
< aj>
bitcoinperf.com ?
< dongcarl>
Yup!
< gmaxwell>
the FP rate for something like that has to be very low, --- otherwise you just getting people making goofy changes to silence the analyizer, and that behavior lowers software quality.
< sipa>
agree; it's probably useful in combination with someone who is familiar with the code enough to determine which issues are relevant
< jamesob>
dongcarl sipa: bitcoinperf's interface needs a lot of work (and there's some of that in progress atm). Down the road I plan on building in some kind of opt-in benchmarking per PR; not necessarily anything that reports back to Github, but that may have PR-specific page on bitcoinperf. Comments to the PR are potentially disruptive given the number of listeners (see drahtbot's quest for one comment per PR).
< dongcarl>
Gotcha
< dongcarl>
Maybe someone can comment “@benchbot plz bench” for opt-in benchmarking or something like that
< jamesob>
Could be, though that's another boilerplate comment. I dunno, haven't thought a ton about it. Think hooking into Github's commit status API a la Travis might be worth exploring too.
< gmaxwell>
anything that runs per PR and dumps data somewhere is great
< gmaxwell>
including crazy sanitizers and whatever
< gmaxwell>
We just should avoid things causing notifications unless we actually want people to take action on the vast majority of to notifications
< dongcarl>
Yeah I agree notifications should be meaningful...
< dongcarl>
Looks like benchmarks and static analysis could be notices with links attached
< luke-jr>
would be nice if GitHub let us add our own buttons to PRs :P
< dongcarl>
luke-jr: well I think each check can have a details url
< luke-jr>
yeah, I just mean if we want some things to run conditionally
< dongcarl>
Oh right yeah
< dongcarl>
Thinking off the top of my head: perhaps we can have tags that activate checks? As in... fanquake-driven development?
< luke-jr>
eh, it might work if anyone could add tags
< dongcarl>
True.
< dongcarl>
Yeah I'm just thinking about developer/reviewer ergonomics... Looks like I need to take a harder look at Github API
< bitcoin-git>
[bitcoin] kallewoof opened pull request #14834: validation: do not break assumption that pindexPrev may be null (master...20181129-contextualcheckblock-pindexprev-nullness) https://github.com/bitcoin/bitcoin/pull/14834
< kallewoof>
dongcarl: you reminded me to run analyzer again. Rewriting those reports was a big pain in the ass though, but I might get around to it.
< dongcarl>
kallewoof: haha yeah I saw that PR and was like: I KNOW WHAT KALLE JUST DID
< kallewoof>
lol!
< kallewoof>
I figured I'd fix the obvious ones, to cut down on the report writing pain :P
< dongcarl>
kallewoof: I'm gunna see if I can find a solution to have more automatic analysis for PRs without spamming useless notifications
< dongcarl>
but yeah stopgap is you lol
< kallewoof>
TBH I think keeping track of which issues have been reported and only reporting them once with a single location where everyone can review all of them would be good enough, presuming FPs are not overly bad.
< kallewoof>
Like.. "This PR introduces a new warning in the Clang Analyzer toolset. Make sure this is a false positive before this is merged. (Note: this message only appears once for this PR.)" or something.
< dongcarl>
True
< gmaxwell>
I think these PRs might also be candidates for 0.17.x backport
< adiabat>
saw sipa's mention of the work I'm doing on accumulators, I don't have anything published yet but if anyone's interested can discuss on #utreexo (or here, but might be off-topic)
< dongcarl>
fanquake: Been on hold for a bit as I've been busy with other stuff... I'll take a look at your rebase for that PR
< promag>
can't attend meeting :(
< promag>
fanquake: the perpetually failing CI << I'm hoping #14670 fixes it
< gmaxwell>
The broken getbalance behavior should get fixed. It's an actual bug.
< wumpus>
that's only broken on 0.17 and not master?
< provoostenator>
#14602
< gribble>
https://github.com/bitcoin/bitcoin/issues/14602 | Bugfix: Correctly calculate balances when min_conf is used, and for getbalance("*") by luke-jr · Pull Request #14602 · bitcoin/bitcoin · GitHub
< achow101>
hi
< wumpus>
wait, I added #14782 which is the 0.17 backport
< gribble>
https://github.com/bitcoin/bitcoin/issues/14782 | [0.17] Bugfix: Correctly calculate balances when min_conf is used, and for getbalance("*") by luke-jr · Pull Request #14782 · bitcoin/bitcoin · GitHub
< jnewbery>
I'm not clear what the behaviour is 'supposed' to be. There's no documentation or test coverage
< gmaxwell>
wumpus: not yet, there is another PR
< wumpus>
why is a backport open before something is merged into master? is it signifiantly different?
< provoostenator>
Ah yes, it's already marked for backport.
< meshcollider>
The 0.17 backport of it is different yes
< meshcollider>
Because of the accounts removal
< sipa>
maybe the getbalance behaviour needs to be topic on its own>?
< wumpus>
right
< gmaxwell>
jnewbery: thats a fine concern, but it's not cool to break functionality just because there wasn't enough tests to stop you from doing so
< meshcollider>
ping luke-jr
< wumpus>
if there's any doubt it probably needs to stay as it was
< gmaxwell>
The alternative of going and reverting the breaking change is unfortunately too disruptive (I already tried).
< meshcollider>
Re-implementing the old behaviour without reintroducing account parameters is the issue
< jnewbery>
the new behaviour in the fix is different from pre-0.17
< meshcollider>
And it has some weird parameter type polymorphism
< wumpus>
well for 0.18, changing the behavior can be discussed if there's agood reason
< sipa>
gmaxwell: well given the removal of accounts, i think it's fair to say that anyone passing not-nothing as the account parameter to getbalance needs to change, and you can get the 0-conf behavior using getunconfirmedbalance
< jnewbery>
so before we decide whether it should be merged, we should figure out what the behavious should be, document it and cover it with tests so it's not regressed
< provoostenator>
getunconfirmedbalance _only_ includes unconfirmed balance afaik
< sipa>
oh
< wumpus>
for 0.17 it'd be good to go back to the old behavior because it was not announced in the release notes that it would change
< sipa>
ignore my suggestion, again
< meshcollider>
Add them together then :)
< gmaxwell>
if you add them you'll double count
< meshcollider>
wumpus: that's why the 0.17 backport was opened already
< provoostenator>
It's useful in practice to be able to get the balance including unconfirmed, so you can compose a PSBT while you're waiting for confirmation.
< BlueMatt>
also for 0.17.1 this seems a bit too late to be figuring out
< BlueMatt>
we can revisit for 0.17.2 if there is one
< provoostenator>
(without doing math)
< BlueMatt>
but 0.17.0 currently cannot be self-built and bitcoin-qt opened on latest fedora/ubuntu release
< BlueMatt>
both of which have been out for a bit now
< gmaxwell>
It isn't just a question of unconfirmed or not, it's a question of your balance suddenly showing zero when you make a small payment.. because now your change is unconfirmed.
< sipa>
gmaxwell: getbalance includes change
< sipa>
just not unconfirmed incoming payments
< sipa>
that was the distinction between getbalance and getbalance *, afaik
< wumpus>
#topic getbalance
< gmaxwell>
Right
< gmaxwell>
sipa: and thats also why you can't add getunconfirmedbalance.
< jnewbery>
for those without context, this is (mostly) my fault for removing accounts. The first parameter to getbalance was 'account', it was marked DEPRECATED with a warning saying 'don't use this'. I removed it and it turns out people were using it to sum up untrusted coins
< sipa>
you can simulate "getbalance *" using "getbalance minconf=1" + "getunconfirmedbalance" i guess
< wumpus>
BlueMatt: yes, let's discuss what to in include in 0.17.1 in the 0.17.1 topic
< meshcollider>
But the actual difference wasn't just unspent, it was untrusted, right?
< sipa>
meshcollider: indeed
< meshcollider>
Unconfirmed*
< jnewbery>
getbalance -minconf=0 includes unconfirmed but not untrusted coins (eg your change)
< jnewbery>
getbalance * 0 would (prior to 0.17) include untrusted
< jnewbery>
so for example if someone sent you an output, then RBF'ed it, you'd still see that in your getbalance * 0
< meshcollider>
as you pointed out on the PR though john, does that also include conflicting transactions and stuff too?
< jnewbery>
or RBF'ed it to you, you'd double count it
< jnewbery>
or you could have a negative balance
< meshcollider>
Thats not sane behaviour to keep then :/
< wumpus>
that sounds pretty broken
< gmaxwell>
I didn't think it included conflicteds, but actually it makes sense.
< jnewbery>
I don't know why people were using getbalance * 0, or what they were using it for. I've asked a few times on the PR but no-one can answer
< gmaxwell>
If you recall when we removed accounts I raised a concern that didn't we still need move to fixed people's negative balances... because we still had those reported from time to time.
< jnewbery>
ok, I've fixed that :)
< gmaxwell>
jnewbery: to see unconfirmed payments... it's certantly what I always did before I gave up on balances and just switched to looking at the unspent outputs directly
< ryanofsky>
It sounded to me like people were just using it to get unconfirmed balance without having two make two calls
< provoostenator>
That's exactly how I use it.
< sipa>
how about adding a "requiretrusted" parameter to getbalance...?
< gmaxwell>
ryanofsky: not just without having to make two calls, because getbalance included 0 conf trusted coins.
< provoostenator>
If you want to prepare a transaction to spend all, you need to enter the amount to send.
< gmaxwell>
ryanofsky: and the minconf parameter was added later
< jnewbery>
I think it's fine to have a balance including unconfirmeds if they're trusted (ie all inputs were yours). Having a balance containing unconfirmeds that aren't trusted and so can be double-accounted or worse doesn't make sense to me
< meshcollider>
sipa: the PR introduces a bool for trustedness
< sipa>
meshcollider: oh!
< jnewbery>
I strongly discourage any 'balance' that contains untrusted coins
< sipa>
does it allow counting untrusted, while excluding conflicting etc?
< meshcollider>
No
< gmaxwell>
jnewbery: I don't understand "trusted and so can be double-accounted" -- even a trusted payment could be conflicted
< provoostenator>
jnewbery: why? Isn't that implied when the user enters 0 instead of 1 confirmations?
< meshcollider>
unconfirmed != untrusted though
< gmaxwell>
jnewbery: we _show_ unconfirmed payments in the wallet, so it's somewhat odd and confusing that we don't have "pending balance"
< wumpus>
the GUI does have a 'unconfirmed balance' IIRC?
< sipa>
showing untrusted balance sounds perfectly reasonable, but it shouldn't include conflicting/... things
< meshcollider>
We could break the getbalance output and return an array of "confirmed":, "unconfirmed":, "conflicting:
< jnewbery>
for me 'pending balance' doesn't make sense if it can double-count conflicting transactions. It's worse that not having a pending balance at all
< meshcollider>
^
< sipa>
jnewbery: i agree
< morcos>
yes, its not clear to me what the problem is with using getbalance and getunconfirmedbalance when using -cli and the GUI already does the right thing?
< gmaxwell>
In any case, you wanted the use case, the reason people use getbalance "*" is to (Attempt) to learn their pending balance. What they'll have assuming nothing goes wrong. Now if that was also double counting conflicteds and ending up negative, I think thats a bug. (and probably the origin of the negative balances I was concerned about before)
< wumpus>
users don't know what 'untrusted balance' is supposed to be, I think exposing these kind of implementation details on the interface is a bit confusing
< provoostenator>
I agree double counting is undesirable.
< jnewbery>
it will make users think their money has disappeared because once one of those conflicting transactions is confirmed their balance goes down
< morcos>
can someone clearly point out exactly what we can not do now (other than we didn't properly explain what was changing)
< meshcollider>
luke-jr: should be here to argue the other side
< sipa>
jnewbery: i think no getbalance* call should evr include conflicted things
< provoostenator>
It should probably use mempool rules to figure out which transaction to ignore.
< sipa>
provoostenator: some things do
< meshcollider>
IMO setting min_conf to 0 is enough?
< gmaxwell>
sipa: wait. I would think that it would include one side of a conflict...
< sipa>
gmaxwell: oh sure; terminology
< wumpus>
isn't it supposed to make one side of a conflict non-conflicting?
< sipa>
i guess by non-conflicting i mean "in the mempool" ?
< wumpus>
right
< gmaxwell>
Good! jnewbery's comment about balances going down confused me
< sipa>
gmaxwell: or do you think unconfirmed untrusted not-in-the-mempool unconflicted things should be counted?
< jnewbery>
sorry, I've got to step away now. My main point is that the expected behaviour should be well documented and covered by tests
< gmaxwell>
sipa: no. I don't.
< sipa>
ok, good
< jnewbery>
I've written a test script for wallet balances, but we need to agree what the behaviour *should* be before merging any of this stuff
< meshcollider>
Agreed
< provoostenator>
If something gets confirmed against what you would expect fromt he mempool then a balance change is fine imo, but not otherwise.
< sipa>
so how about getbalance requires the minconf as directed, and (if trusted is directed, trusted, otherwise in-the-mempool)
< provoostenator>
sipa: the only criteria for "trusted" is whether it's change, right?
< gmaxwell>
I think if sipa is right that the behavior can be emulated by getbalance minconf=1 + getunconfirmed then at least we have a workaround. Though it's always confusing to have a deault behavior which cannot be explained by some setting of the arguments
< sipa>
provoostenator: or confirmed
< gmaxwell>
"What is the default minconf setting of getbalance? Mu. getbalance output shows minconf=0 for trusted outputs, and minconf=1 for others. so trusted defaults to true? No, there is no trusted argument."
< sipa>
gmaxwell: #14602 includes a trusted_only argument that is true by default
< gribble>
https://github.com/bitcoin/bitcoin/issues/14602 | Bugfix: Correctly calculate balances when min_conf is used, and for getbalance("*") by luke-jr · Pull Request #14602 · bitcoin/bitcoin · GitHub
< provoostenator>
I know the wallet makes a distinction between change and non-change when it comes to spending, which is fine, but I don't think we should do that for balance. So by default minconf=0 should show change and non-change, but not replaced-by.
< ryanofsky>
just want to note, it looks like we didn't delete the GetLegacyBalance function, so it would be trivial to restore the old getbalance "*" behavior, if we can't figure out something better right now
< gmaxwell>
I know 14602 does. I'm trying to imagine a world where we keep things as they are and try to release note ourselves out of it
< provoostenator>
(as for 0.17 I'm fine with release-noting our way of it)
< sipa>
oh, i was just trying to establish what the desired behavior is before we go discuss what to do about it
< provoostenator>
sipa: that's a good idea
< sipa>
and i think there is at least a rough conclusion? allow counting untrusted, but never count not-in-mempool?
< gmaxwell>
Right, I think the desired behavior is that balances shows confirmed + trusted-in-mempool-0conf. And that you be able to adjust the threshold for confirmed and turn the trusted-in-mempool-zeroconf on and off. And if you make the confirmed=0, it should still only count in-mempool
< provoostenator>
(and maybe add a 4th argument along the lines of unconfirmed_change_only
< gmaxwell>
(and potentially the arguments should just be minconf_for_not_you=1 minconf_for_from_you=0 )
< sipa>
i think the default of minconf=0 but trusted_only=true makes sense; if you choose trusted_only=false it should still only include confirmed or mempool things
< morcos>
sipa: the problem with never counting not-in-mempool is if you spend a 10 BTC input to pay 0.001 BTC, and that tx doesn't go in your mempool then your balance goes down by 10 mysteriously, but thats hard to solve
< sipa>
morcos: yeah...
< gmaxwell>
morcos: yes, I think thats hard to solve, also it's kinda good that it goes down in that you're actually screwed for the moment :P
< gmaxwell>
like if that txn doesn't confirm, your funds are actually stuck.
< gmaxwell>
(until you abandon the transaction)
< wumpus>
right
< gmaxwell>
I guess I can argue the consequences either way, I can imagine someone losing a wallet file with lots of bitcoin in it because it showed a low balance due to some easily abandoned transactions.
< sipa>
so i think we can work out how to implement this for 0.17 and 0.18 on the PRs or issues; no need for further design here?
< gmaxwell>
I thought luke's PR basically implemented what we came up with here. But I'm not sure.
< sipa>
gmaxwell: i'm not sure about the mempool part
< wumpus>
gmaxwell: people are likely to use 'getbalance' without parameters in that case, to check wallet balance, not the specific combo that this is about
< gmaxwell>
certantly if we're showing all unconfirmeds, including both sides of a conflict, thats bad. :P
< provoostenator>
Luke's PR adds a trusted_only as a first parameter. I find that a fairly confusing term, so hopefully we can avoid it with the ^
< sipa>
with the what?
< wumpus>
trusted_only sounds really confusing to users, if you make a parameter like that you need to carefully explain in the RPC help what 'truste'd means in this setting
< sipa>
agree, it needs documentation for sure
< wumpus>
it's far from obvious, I don't know if we picked a good term for it
< provoostenator>
With what you said: "allow counting untrusted, but never count not-in-mempool"
< provoostenator>
It's already a default, so maybe just a matter of renaming and documenting it.
< sipa>
provoostenator: it can certainly be explained more friendly as "Whether unconfirmed payments from others are excluded"
< BlueMatt>
provoostenator: also there are a number of other bugs and backports that are done, etc
< sipa>
we have a bunch of bugfixes lined up too
< wumpus>
nah let's do a proper release
< sipa>
it may be time for a 0.17.1
< BlueMatt>
things already merged on master probably worth backporting
< wumpus>
it's too late for 0.17.0.2 IMO
< BlueMatt>
cause there are a bunch there
< provoostenator>
Right, that would involve branching off of the 0.17.0.1 tag and get weird.
< wumpus>
.z releases are really 'oops we did a release and need to fix this thing up'
< BlueMatt>
but, at least IMO, its super annoying that you cant build+run bitcoin-qt on the now-several-months-old versions of various OSs, and I presume other rolling distros (eg arch) also have this issue
< wumpus>
not for month later
< instagibbs>
I keep crashing, get 14380 in please ;P
< BlueMatt>
so I would prefer we not wait around for fixing getbalance :p
< wumpus>
I agree with BlueMatt on this
< BlueMatt>
#14380 looks more than merge-able from where I'm sitting in the ack department
< gribble>
https://github.com/bitcoin/bitcoin/issues/14380 | fix assert crash when specified change output spend size is unknown by instagibbs · Pull Request #14380 · bitcoin/bitcoin · GitHub
< BlueMatt>
or just generally a "lets take things that we can merge today, do backports, and get this thing going"
< provoostenator>
BlueMatt: "IRC logs have been disabled due to the EU General Data Protection Regulation (GDPR)."
< wumpus>
BlueMatt: +1
< provoostenator>
+1
< wumpus>
#topic proposed meeting topics ahead of time (moneyball)
< moneyball>
I’d like to suggest that people can propose Core weekly meeting topics anytime throughout the week in this IRC channel using #proposedmeetingtopic tag. I will collect these throughout the week, add them to a gist that anyone can have a look at during the week. The primary intended benefit of this is to allow people to know which topics are going to be discussed ahead of time allowing them to prepare.
< moneyball>
Any concerns?
< BlueMatt>
seems reasonable
< provoostenator>
+1
< BlueMatt>
easier than remembering to mention them in meeting
< Chris_Stewart_5>
+1
< meshcollider>
+1
< BlueMatt>
next topic? #thatwaseasy
< wumpus>
yes, good idea, let's see how it goes
< moneyball>
Ok I will start doing it!
< wumpus>
BlueMatt: ok what was your proposal, I'm not in #bitcoin-dev
< provoostenator>
And there's no logs..
< BlueMatt>
oh, i meant bitcoin-dev ml but it hasnt passed moderation yet
< BlueMatt>
anyway, I can tl;dr it in two lines
< BlueMatt>
in lightning (and other related payment channel-y systems) you have the general issue of pre-signing transactions
< BlueMatt>
so you have to predict what the feerate on the network will be in the future at the time your counterparty misbehaves
< BlueMatt>
instead, we'd (obviously) like to use cpfp
< BlueMatt>
but there are the cpfp rbf-pinning-ish issues where your counterparty can fill up the package with big-bug-low-ish-feerate txn
< BlueMatt>
so that you cant cpfp it yourself
< BlueMatt>
I'd like to propose we tweak the cpfp/rbf rules
< BlueMatt>
The last transaction which is added to a package of dependent transactions in the mempool must:
< BlueMatt>
* Have no more than one unconfirmed parent,
< BlueMatt>
* Be of size no greater than 1K in virtual size.
< BlueMatt>
(we'd, in practice, first decrease the package size limits by 1 1K-virtual-size transaction, then make it so that the last one must meet those rules)
< BlueMatt>
this allows for cpfp by letting each side of a lightning channel independantly cpfp without considering what the other side may be doing
< BlueMatt>
well, luckily only one-deep package relay and only for small-ish packages
< BlueMatt>
or at least small dependents
< instagibbs>
sorry how does cpfp get pinned again?
< instagibbs>
RBF pinning I know
< BlueMatt>
instagibbs: A -> B where B is 100KB but low feerate, but high fee. its the same issue for lightning
< BlueMatt>
cause you cant add to the package cause the package is max size
< instagibbs>
ah
< sipa>
my response to this is "eh... sure.... but how?"
< instagibbs>
so it's not something you can attach B'
< BlueMatt>
so you have to add to the package by rbf which means you have to increase total size, if you can even do that, which you'd have to tweak rbf rules to let you replace B with C, but you cant do that today
< BlueMatt>
sipa: hmm?
< sipa>
you need package relay...
< sipa>
that's not just a small few-lines policy change
< provoostenator>
An alternative, I forgot what the problem was, is to drop RBF rule "3. The replacement transaction pays an absolute fee of at least the sum paid by the original transactions." and only consider fee rate of the replacement, regardless of size.
< sdaftuar>
so i think package relay is a whole other topic
< BlueMatt>
I have another bitcoin-dev post coming discussing package relay, but we have a few options there, if we just want to solve package relay for contracting applications like lightning, we can do so with limited consideration, I think
< BlueMatt>
if we want a more general solution I still think its possible but requires more in-depth ATMP refactoring
< sipa>
and P2P changes...
< BlueMatt>
assuming we do at least one of those two, this is a simple (I think) policy change that I'm hoping people here dont object to
< BlueMatt>
possibly, though actaully not neccessarily
< BlueMatt>
but, probably
< sipa>
this sounds very preliminary to me
< BlueMatt>
i presume you mean the package relay part?
< BlueMatt>
the policy rule here is pretty straightforward, I think?
< BlueMatt>
and not very preliminary, again assuming some form of package relay
< sipa>
i don't understand how you can say it's not preliminary, while relying on something we have no idea about yet
< meshcollider>
Shall we discuss next week instead after reading the ml
< gmaxwell>
MAGIC
< BlueMatt>
heh, sdaftuar and I have been batting package relay back and forth for months, but, whatever
< BlueMatt>
agreed we need a harder proposal for package relay
< sdaftuar>
well i think it'd be useful to layout the design goals for a package relay ssytem
< BlueMatt>
anyway, yes, we'll discuss package relay on the ml and then come back to this
< sipa>
BlueMatt: yes i'd like to see a proposal for that :)
< gmaxwell>
BlueMatt: the point is that the policy change justification is a black box to us for the moment. :P
< BlueMatt>
also what sdaftuar said
< BlueMatt>
this is also a motivator for package relay
< BlueMatt>
as its yet another thing to feed into the design goals
< BlueMatt>
gmaxwell: I mean if you presume package relay it isnt at all a black box?
< instagibbs>
sdaftuar, you ahve 3 minutes to resolve it
< wumpus>
#topic wallet maintainer
< wumpus>
meshcollider has shown interest in becoming wallet maintainer!
< provoostenator>
Awesome
< * sipa>
offers condolences
< sipa>
:)
< provoostenator>
Anyoe against, no, done. End meeting :-)
< wumpus>
which is great news, I think he'd be good at his, many of his his contributions have been to the wallet
< meshcollider>
:)
< wumpus>
and he's been active contributor for quite a while
< sipa>
more seriously, sounds great to me
< achow101>
+1
< wumpus>
yes!
< bitcoin-git>
[bitcoin] vim88 opened pull request #14846: Docs: Adds development guidelines about Scripts shebang to developer-notes.md. (master...add_scripts_development_guidelines) https://github.com/bitcoin/bitcoin/pull/14846
< wumpus>
ok, that was a quick topic, was intending to bring it up sooner but BlueMatt kind of ninja'd it :)
< wumpus>
#endmeeting
< lightningbot>
Meeting ended Thu Nov 29 20:00:46 2018 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
< provoostenator>
So have we figured out what a mainainter actually does? Shepharding PR's on the topic and some sort of super ACK?
< meshcollider>
I don't want a super ACK ;)
< provoostenator>
By super ACK I mean that you would consider it reviewed by enough compentent people.
< wumpus>
he decides when to merge the stuff
< provoostenator>
More of a meta-ACK
< BlueMatt>
no, definitely no such thing as super ack
< BlueMatt>
super-concept-ack, though, yes :)
< wumpus>
"consider it reviewed by enough compentent people", that's more or less the idea yes
< wumpus>
understand the changes where possible
< provoostenator>
That sounds good.
< wumpus>
unless it's coin selection he opted out of that :)
< meshcollider>
^ lol
< achow101>
coin selection? what's wrong with that?
< achow101>
it's super simple and not at all hard to understand :p
< meshcollider>
Trivial even :p
< provoostenator>
You get a coin, you get a coin, everyone gets a coin!
< sipa>
we'll just ask Bob who gets which coin
< BlueMatt>
sipa: it was pointed out that the above policy doesnt, entirely rely on package relay - eg if you have a transaction A that you want to confirm, today, you pay a feerate that is reasonable for today and hope that that's enough to get confirmed later...well obviously if it isnt maybe you're still gonna meet the get-into-mempool limit even if not get-mined-limit
< BlueMatt>
so its still a significant improvement over today
< jnewbery>
sorry, missed the end of the meeting, but big ACK for meshcollider being wallet maintainer 🎉
< luke-jr>
I can very well imagine the definition of trusted being revised in the future
< zallarak>
Is there any sort of prioritization among open issues? I picked one up to start with, and now in finding the next one to work on, was wondering how to choose.
< hebasto>
wumpus: hi, regarding #14409 "This absolutely needs testing in a functional test." Should I submit another PR for this functional test or should it be as another commit in the same PR?