1 2017-01-18 01:27:35 0|bitcoin-git|[13bitcoin] 15pinheadmz opened pull request #9571: RPC: getblockchaininfo returns BIP signaling statistics (06master...06master) 02 https://github.com/bitcoin/bitcoin/pull/9571
2 2017-01-18 07:02:53 0|jonasschnelli|BlueMatt: I'm working on the fixes for #9461
3 2017-01-18 07:03:59 0|gribble|https://github.com/bitcoin/bitcoin/issues/9461 | [Qt] Improve progress display during headers-sync and peer-finding by jonasschnelli ÷ Pull Request #9461 ÷ bitcoin/bitcoin ÷ GitHub
4 2017-01-18 07:55:44 0|bitcoin-git|[13bitcoin] 15jl2012 opened pull request #9572: Skip witness sighash cache for non-segwit transactions (06master...06nocache) 02 https://github.com/bitcoin/bitcoin/pull/9572
5 2017-01-18 09:56:48 0|bitcoin-git|13bitcoin/06master 1495bab82 15practicalswift: Remove unused Python imports
6 2017-01-18 09:56:48 0|bitcoin-git|[13bitcoin] 15MarcoFalke pushed 2 new commits to 06master: 02 https://github.com/bitcoin/bitcoin/compare/6696b4635ceb...b0b57a17306a
7 2017-01-18 09:56:49 0|bitcoin-git|13bitcoin/06master 14b0b57a1 15MarcoFalke: Merge #9508: Remove unused Python imports...
8 2017-01-18 09:57:03 0|bitcoin-git|[13bitcoin] 15MarcoFalke closed pull request #9508: Remove unused Python imports (06master...06remove-unused-python-import) 02 https://github.com/bitcoin/bitcoin/pull/9508
9 2017-01-18 11:37:17 0|bitcoin-git|[13bitcoin] 15fanquake opened pull request #9574: [depends] Fix QT build on OSX (06master...06fix-osx-depends-build) 02 https://github.com/bitcoin/bitcoin/pull/9574
10 2017-01-18 14:15:14 0|BlueMatt|cfields: yo
11 2017-01-18 14:16:15 0|BlueMatt|where are we on #9212 and #9278? The fix for 9212 at https://github.com/bitcoinfibre/bitcoinfibre/commit/8e2c2cf418adb3dad1479c3f8890e0a2c8b6f709 has been in production for a while (albeit not on nodes with a ton of connection churn) so I'm reasonably confident in it, but iirc you were not a fan?
12 2017-01-18 14:16:17 0|gribble|https://github.com/bitcoin/bitcoin/issues/9212 | Assertion failed: (nSendVersion != 0), function GetSendVersion, file ./net.h, line 775. ÷ Issue #9212 ÷ bitcoin/bitcoin ÷ GitHub
13 2017-01-18 14:16:17 0|gribble|https://github.com/bitcoin/bitcoin/issues/9278 | test_bitcoin fails valgrind ÷ Issue #9278 ÷ bitcoin/bitcoin ÷ GitHub
14 2017-01-18 14:18:21 0|BlueMatt|for those bored, #9392 should be an easy fix
15 2017-01-18 14:18:22 0|gribble|https://github.com/bitcoin/bitcoin/issues/9392 | Wallet ancestor sanity-check ignores sigops ÷ Issue #9392 ÷ bitcoin/bitcoin ÷ GitHub
16 2017-01-18 16:15:35 0|cfields|BlueMatt: I didn't like that change at the time because 1. We weren't always disconnecting (or checking fDisconnect) as necessary at the time, and 2. It changes the "you can only send 1 version message" semantics a bit. 1. should be fixed since fedea8a14. I think your change is probably ok, but I think we should think through 2 a little.
17 2017-01-18 16:17:39 0|BlueMatt|I know we also discussed moving off of the "nVersion is set means connected" to using the fSuccessfullyConnected flag again
18 2017-01-18 16:17:46 0|BlueMatt|they're kinda confused and redundant atm
19 2017-01-18 16:18:40 0|cfields|yea
20 2017-01-18 16:19:15 0|cfields|that would be my preference, only issue there is defining "successfully connected"
21 2017-01-18 16:23:28 0|cfields|BlueMatt: as a weird example, I think your change would allow for sending infinite version messages in which the addrMe portion fails to deserialize
22 2017-01-18 16:23:58 0|cfields|so a peer could change their nVersion a bunch of times before locking it in
23 2017-01-18 16:24:17 0|cfields|(i don't know what good that would do, but we certainly shouldn't be allowing that)
24 2017-01-18 16:24:45 0|BlueMatt|I'd file that under "undefined behavior" I believe in that case we may be required to make a best effort to eat the sender's cat
25 2017-01-18 16:25:19 0|BlueMatt|but short that, I'm not sure if we care that they can do that?
26 2017-01-18 16:25:33 0|BlueMatt|as long as we actually gate on fSuccessfullyConnected
27 2017-01-18 16:25:54 0|cfields|BlueMatt: right, that was my point. atm we don't in many places
28 2017-01-18 16:25:55 0|BlueMatt|cfields: do you have time to look at that, or should I just whip something up with fSuccessfullyConnected after I finish review of one or two others?
29 2017-01-18 16:26:09 0|BlueMatt|well atm we check nVersion != 0 pretty much everywhere, i think
30 2017-01-18 16:26:15 0|BlueMatt|as a proxy for connectedness
31 2017-01-18 16:27:02 0|cfields|BlueMatt: right, and in the above scenario, we don't consider them connected, but their nVersion is set.
32 2017-01-18 16:27:23 0|BlueMatt|ok, so replace nVersion != 0 with fSuccessfullyConnected everywhere, i guess?
33 2017-01-18 16:29:08 0|cfields|i think so.
34 2017-01-18 16:29:14 0|BlueMatt|k
35 2017-01-18 16:29:19 0|cfields|and at this point, we can remove the assert
36 2017-01-18 16:29:21 0|BlueMatt|I'll do it after lunch if you havent gotten to it
37 2017-01-18 16:29:26 0|BlueMatt|I'd prefer leaving it in?
38 2017-01-18 16:29:33 0|BlueMatt|afaict its super easy to fix
39 2017-01-18 16:29:38 0|BlueMatt|I'm incredibly confident in the above change
40 2017-01-18 16:29:49 0|BlueMatt|(as its equivalent to the change I've been running for months now)
41 2017-01-18 16:30:20 0|cfields|BlueMatt: point me at a node running it, let's see if i can bring it down :)
42 2017-01-18 16:31:01 0|BlueMatt|the fibre nodes
43 2017-01-18 16:31:02 0|BlueMatt|:p
44 2017-01-18 16:31:48 0|cfields|heh
45 2017-01-18 16:34:03 0|BlueMatt|I'll swap my public node to master soonish
46 2017-01-18 16:34:08 0|BlueMatt|it gets a shitload of connection churn
47 2017-01-18 16:34:18 0|BlueMatt|and fun spy nodes and shit which do batshit crazy things
48 2017-01-18 16:52:10 0|bitcoin-git|[13bitcoin] 15practicalswift opened pull request #9575: [trivial] Add comment about unreachable code (06master...06never-executed-comment) 02 https://github.com/bitcoin/bitcoin/pull/9575
49 2017-01-18 16:52:31 0|BlueMatt|jonasschnelli: https://github.com/bitcoin/bitcoin/pull/9461#discussion_r96680764
50 2017-01-18 17:01:37 0|bitcoin-git|[13bitcoin] 15practicalswift opened pull request #9576: [wallet] Remove redundant initialization (06master...06remove-redundant-initialization-ii) 02 https://github.com/bitcoin/bitcoin/pull/9576
51 2017-01-18 17:26:08 0|cfields|BlueMatt: ok, whipping something up on top of yours. Think I've got a full picture of it all in my head.
52 2017-01-18 17:36:47 0|BlueMatt|cfields: ok, thanks, got distracted on other things
53 2017-01-18 18:38:57 0|BlueMatt|someone should tag #9569, #9371 and #9148 for 0.14 because they fix 0.14-tagged issues
54 2017-01-18 18:38:58 0|gribble|https://github.com/bitcoin/bitcoin/issues/9569 | Setting -blocksonly sets -maxmempool to zero. by jnewbery ÷ Pull Request #9569 ÷ bitcoin/bitcoin ÷ GitHub
55 2017-01-18 18:39:00 0|gribble|https://github.com/bitcoin/bitcoin/issues/9371 | Notify on removal by morcos ÷ Pull Request #9371 ÷ bitcoin/bitcoin ÷ GitHub
56 2017-01-18 18:39:01 0|gribble|https://github.com/bitcoin/bitcoin/issues/9148 | Wallet RPCs can return stale info due to ProcessNewBlock Race ÷ Issue #9148 ÷ bitcoin/bitcoin ÷ GitHub
57 2017-01-18 18:58:21 0|BlueMatt|sorry, that last one should be #9570
58 2017-01-18 18:58:23 0|gribble|https://github.com/bitcoin/bitcoin/issues/9570 | Block Wallet RPCs until wallet is synced to our current chain by TheBlueMatt ÷ Pull Request #9570 ÷ bitcoin/bitcoin ÷ GitHub
59 2017-01-18 19:06:19 0|bitcoin-git|[13bitcoin] 15laanwj pushed 7 new commits to 06master: 02 https://github.com/bitcoin/bitcoin/compare/b0b57a17306a...6012967c4746
60 2017-01-18 19:06:20 0|bitcoin-git|13bitcoin/06master 14843c560 15Pieter Wuille: Avoid unaligned access in crypto i/o
61 2017-01-18 19:06:20 0|bitcoin-git|13bitcoin/06master 14f94f3e0 15Pieter Wuille: Avoid integer overflows in scriptnum tests
62 2017-01-18 19:06:21 0|bitcoin-git|13bitcoin/06master 146b03bfb 15Pieter Wuille: Fix memory leak in wallet tests
63 2017-01-18 19:06:36 0|bitcoin-git|[13bitcoin] 15laanwj closed pull request #9512: Fix various things -fsanitize complains about (06master...06sanitize) 02 https://github.com/bitcoin/bitcoin/pull/9512
64 2017-01-18 19:08:34 0|Chris_Stewart_5|Does some one mind merging in #9350? Pretty trivial, just fixing some documentation on tests
65 2017-01-18 19:08:36 0|gribble|https://github.com/bitcoin/bitcoin/issues/9350 | [Trivial] Adding label for amount inside of tx_valid/tx_invalid.json by Christewart ÷ Pull Request #9350 ÷ bitcoin/bitcoin ÷ GitHub
66 2017-01-18 19:21:26 0|morcos|wumpus: sipa: I updated #9371 in a way that I think accomplish both of our goals
67 2017-01-18 19:21:28 0|gribble|https://github.com/bitcoin/bitcoin/issues/9371 | Notify on removal by morcos ÷ Pull Request #9371 ÷ bitcoin/bitcoin ÷ GitHub
68 2017-01-18 19:34:52 0|cfields|BlueMatt: hmm, looks like that could make your recent-tx cache significantly smarter
69 2017-01-18 19:38:08 0|BlueMatt|yes, lots of ways it could be made smarter :)
70 2017-01-18 19:57:22 0|bitcoin-git|[13bitcoin] 15jnewbery opened pull request #9577: Fix docstrings in qa tests (06master...06docstrings) 02 https://github.com/bitcoin/bitcoin/pull/9577
71 2017-01-18 19:58:05 0|BlueMatt|sipa_/wumpus: so I think the only way to fix the regression introduced in #7946 without giving up the gains it gave us is to introduce a second cs_wallet - cs_wallet_locked_before_cs_main
72 2017-01-18 19:58:07 0|gribble|https://github.com/bitcoin/bitcoin/issues/7946 | Reduce cs_main locks during ConnectTip/SyncWithWallets by jonasschnelli ÷ Pull Request #7946 ÷ bitcoin/bitcoin ÷ GitHub
73 2017-01-18 19:58:15 0|BlueMatt|see https://github.com/bitcoin/bitcoin/pull/9570#issuecomment-273583506
74 2017-01-18 19:58:18 0|BlueMatt|please dont kill me
75 2017-01-18 20:01:38 0|cfields|morcos: ah, i missed your comment in the description. I guess i'm requesting choice #1 :)
76 2017-01-18 20:01:40 0|gribble|https://github.com/bitcoin/bitcoin/issues/1 | JSON-RPC support for mobile devices ("ultra-lightweight" clients) ÷ Issue #1 ÷ bitcoin/bitcoin ÷ GitHub
77 2017-01-18 20:02:08 0|BlueMatt|cfields: regarding SyncTransaction changes, see https://github.com/bitcoin/bitcoin/pull/9570/commits/bc4c4c66863fce718402a3f9b2da4e92c1898745
78 2017-01-18 20:02:20 0|BlueMatt|I do remove the NOT_IN_BLOCK kludge ther
79 2017-01-18 20:02:21 0|BlueMatt|e
80 2017-01-18 20:02:30 0|BlueMatt|(but this introduces the need for a second wallet lock)
81 2017-01-18 20:02:54 0|morcos|cfields: I'm trying to do the minimal required in 9371 to fix the regression though.. as it needs to be merged for 0.14...
82 2017-01-18 20:03:24 0|cfields|morcos: ok, fair enough
83 2017-01-18 20:03:31 0|morcos|so i don't want to do a new signal now b/c it will require thinking carefully about what should subscribe to it
84 2017-01-18 20:03:50 0|morcos|but i'm fine dropping the SYNC_TRANASCTION_NOT_IN_BLOCK as BlueMatt did
85 2017-01-18 20:04:28 0|cfields|ok, will look over that one too
86 2017-01-18 20:10:32 0|sipa_|BlueMatt: i'm unclear about it... can you write a simple commit that does the cs_wallet_locked_before_cs_main ?
87 2017-01-18 20:10:48 0|BlueMatt|sipa_: I think we can push off to 0.15, but I will do that, yes, one sec
88 2017-01-18 21:07:39 0|luke-jr|done with reviews I think. ping me if there's anything needed priority on before 0.14 freeze.
89 2017-01-18 21:45:14 0|BlueMatt|sipa_: see https://github.com/TheBlueMatt/bitcoin/commit/c99e4d107aac573e8de892d7592b04c685186e44
90 2017-01-18 21:45:58 0|BlueMatt|specifically, this half-reverts #9570 to get to the behavior intended in #7946 without introducing the getbalance-etc-may-return-data-from-mid-block-processing regression
91 2017-01-18 21:46:00 0|gribble|https://github.com/bitcoin/bitcoin/issues/9570 | Block Wallet RPCs until wallet is synced to our current chain by TheBlueMatt ÷ Pull Request #9570 ÷ bitcoin/bitcoin ÷ GitHub
92 2017-01-18 21:46:02 0|gribble|https://github.com/bitcoin/bitcoin/issues/7946 | Reduce cs_main locks during ConnectTip/SyncWithWallets by jonasschnelli ÷ Pull Request #7946 ÷ bitcoin/bitcoin ÷ GitHub
93 2017-01-18 22:04:41 0|BlueMatt|someone wanna kick https://travis-ci.org/bitcoin/bitcoin/builds/193151086 ? looks like it failed due to travis timeout because it built dependancies and not because it failed
94 2017-01-18 22:31:21 0|BlueMatt|https://github.com/bitcoin/bitcoin/blob/master/src/txmempool.cpp#L555 <-- that is wrong...100 block reorg and it could blow up (with the checkmempool debug instrumentation enabled)
95 2017-01-18 22:55:27 0|bitcoin-git|[13bitcoin] 15TheBlueMatt opened pull request #9578: Add missing mempool lock for CalculateMemPoolAncestors (06master...062017-01-fix-missing-wallet-mempool-lock) 02 https://github.com/bitcoin/bitcoin/pull/9578
96 2017-01-18 23:10:25 0|cfields|BlueMatt: ok, i've been staring at this for most of the day and i'm reasonably satisfied now. I'm good with your change with 2 tweaks
97 2017-01-18 23:11:15 0|cfields|1. deserialize the whole thing locally. Same change you made for nVersion, let's go ahead and do that for everything. That way we don't end up in a weird state if something throws
98 2017-01-18 23:11:51 0|cfields|and 2. just a quick reordering that moves the assignment into CNode up a bit, and sets nVersion/fSuccessfullyConnected last.
99 2017-01-18 23:17:00 0|cfields|BlueMatt: something like https://github.com/theuni/bitcoin/commit/ed60b005fa0b61ab4454e075e6c7d9a54e2c24a0 on top of yours