< ryanofsky>
jtimon, ok i'll just create little prs for those, sorry for not being clear
< jtimon>
ryanofsky: great, although you were clear, I just didn't had the chance to fix the nits
< BlueMatt>
jtimon: yes?
< BlueMatt>
jtimon: after the first commit on #9494 mapMultiArgs is access without locks while being updated, this is a bug, but its fixed two commits later
< gribble>
https://github.com/bitcoin/bitcoin/issues/9494 | Introduce an ArgsManager class encapsulating cs_args, mapArgs and mapMultiArgs by jtimon · Pull Request #9494 · bitcoin/bitcoin · GitHub
< BlueMatt>
jtimon: its not a huge deal, but it would be nice to not do that
< jtimon>
BlueMatt: oh, I see, got you now, thanks for explaining
< jtimon>
trivial to fix, no problem, will eat first though
< instagibbs>
morcos, what interaction are you worried about exactly re:#10360? The Core wallet already aggressively picks lots of small inputs. My PR wasn't an attempt to fix this degenerate coin selection strategy.
< gribble>
https://github.com/bitcoin/bitcoin/issues/10360 | [WIP] [Wallet] Target effective value during transaction creation by instagibbs · Pull Request #10360 · bitcoin/bitcoin · GitHub
< morcos>
instagibbs: correct that the current algorithm is bad also.. and its possibly 10360 is better, but my concern is now there are more "small" inputs b/c they are effective size inputs.
< instagibbs>
ah... I see
< instagibbs>
however, previously we would pick even negative effective value inputs. Maybe not apples/oranges.
< morcos>
i don't have any code, yet, i was just working out a design... but in looking at the code today.. i feel like it would behoove us to clean up some stuff first...
< morcos>
instagibbs: agreed, thats why i left it as a question mark
< instagibbs>
Yeah it's a good point.
< morcos>
for instance all the nBytes calculation in coincontroldialog could hopefully be abstracted away into the same DummySignTx
< morcos>
back in 2
< morcos>
ok back. or if we are now caching delta nBytes (to include as input) in the wallets spendable outputs themselves, then possibly we just add up those numbers
< morcos>
i started thinking about this, b/c i didnt' like the way some of your changes were duplicating code that had previously been unduplicated.. like the maxTxFee check
< morcos>
anyway.. in my ideal world.. we'd refactor to clean up a bit first... and then start making changes
< morcos>
some of your commits do that
< MarcoFalke>
wumpus: The doxygen docs are last generated in April. Mind to kick the server?
< instagibbs>
morcos, missed the qt stuff. I think additional refactoring is fine if it makes it easier to manage long-term. I'll reply in thread.
< gribble>
https://github.com/bitcoin/bitcoin/issues/9494 | Introduce an ArgsManager class encapsulating cs_args, mapArgs and mapMultiArgs by jtimon · Pull Request #9494 · bitcoin/bitcoin · GitHub
< cfields>
jtimon: doesn't really matter either way. It just stood out as odd
< jtimon>
yeah, I mean, not doing it would be less disruptive, but I guess doing it is "more correct"(tm)
< cfields>
jtimon: I only mentioned it because it looks like it was changed for a reason i was missing. If not, may as well leave it as a const ref, no?
< cfields>
jtimon: i see. I didn't realize you'd already changed it once
< cfields>
jtimon: it's really not worth the time we've put into it :). either way is fine
< jtimon>
cfields: fair enough, I'll leave it as it is now then
< cfields>
sounds good
< bitcoin-git>
[bitcoin] TheBlueMatt opened pull request #10372: Add perf counter data to GetStrongRandBytes state in scheduler (master...2017-05-scheduler-rng) https://github.com/bitcoin/bitcoin/pull/10372
< BlueMatt>
sipa: you said after 10338.... ^
< BlueMatt>
sipa: do you happen to have any idea why there is the huge jump at around 35/37% progress in #10195?
< bitcoin-git>
[bitcoin] MarcoFalke closed pull request #10365: [tests] increase timeouts in sendheaders test (master...improve_sendheaders) https://github.com/bitcoin/bitcoin/pull/10365
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #10374: qa: Warn when specified test is not found (master...Mf1705-qaWarnTestNotFound) https://github.com/bitcoin/bitcoin/pull/10374
< bitcoin-git>
[bitcoin] practicalswift opened pull request #10378: Rename TxConfirmStats to CTxConfirmStats to achieve class naming consistency (master...TxConfirmStats) https://github.com/bitcoin/bitcoin/pull/10378
< cfields>
sipa: out of curiosity, what are your CTxInUndo serialization plans mentioned in 10195?
< sipa>
cfields: create a VectorSerializer<typename E, typename ItemSerializer>(std::vector<E>& v), which instantiates an ItemSerializer<E>(x) for every x in v, and serializes with it
< cfields>
some of those serializations are pretty clunky, but I'll ignore if you intend to replace
< sipa>
the default std::vector serializer would just use a dummy ItemSerializer