12020-07-02T00:00:02 *** bitprophet1 has quit IRC
22020-07-02T00:02:26 *** bitcoin-git has joined #bitcoin-core-dev
32020-07-02T00:02:27 <bitcoin-git> [bitcoin] MarcoFalke opened pull request #19429: test: Fix intermittent failure in wallet_encryption (master...2007-testFixInt) https://github.com/bitcoin/bitcoin/pull/19429
42020-07-02T00:02:27 *** bitcoin-git has left #bitcoin-core-dev
52020-07-02T00:22:07 *** tummy has joined #bitcoin-core-dev
62020-07-02T00:24:14 *** mol has quit IRC
72020-07-02T00:35:27 *** promag has quit IRC
82020-07-02T00:37:41 *** bitcoin-git has joined #bitcoin-core-dev
92020-07-02T00:37:42 <bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/501203aa9101...d6fe5b28dff4
102020-07-02T00:37:42 <bitcoin-git> bitcoin/master fa23fbb MarcoFalke: ci: Run all tests on native mac again
112020-07-02T00:37:43 <bitcoin-git> bitcoin/master d6fe5b2 MarcoFalke: Merge #19427: ci: Run all tests on native mac again
122020-07-02T00:37:44 *** bitcoin-git has left #bitcoin-core-dev
132020-07-02T00:38:02 *** bitcoin-git has joined #bitcoin-core-dev
142020-07-02T00:38:02 <bitcoin-git> [bitcoin] MarcoFalke merged pull request #19427: ci: Run all tests on native mac again (master...2007-ciMac) https://github.com/bitcoin/bitcoin/pull/19427
152020-07-02T00:38:03 *** bitcoin-git has left #bitcoin-core-dev
162020-07-02T00:51:52 *** proofofkeags has quit IRC
172020-07-02T00:52:26 *** proofofkeags has joined #bitcoin-core-dev
182020-07-02T00:52:58 *** Relis has quit IRC
192020-07-02T00:56:41 *** proofofkeags has quit IRC
202020-07-02T01:12:16 *** justanotheruser has quit IRC
212020-07-02T01:29:51 *** proofofkeags has joined #bitcoin-core-dev
222020-07-02T01:32:51 *** DeanWeen has quit IRC
232020-07-02T01:34:30 *** proofofkeags has quit IRC
242020-07-02T01:49:47 *** proofofkeags has joined #bitcoin-core-dev
252020-07-02T01:52:16 *** DeanGuss has joined #bitcoin-core-dev
262020-07-02T01:53:21 *** mol has joined #bitcoin-core-dev
272020-07-02T01:58:01 *** justanotheruser has joined #bitcoin-core-dev
282020-07-02T02:03:19 *** proofofkeags has quit IRC
292020-07-02T02:17:15 *** DeanGuss has quit IRC
302020-07-02T02:22:00 *** DeanGuss has joined #bitcoin-core-dev
312020-07-02T02:23:20 *** Highway61 has quit IRC
322020-07-02T02:27:10 *** andrewtoth_ has quit IRC
332020-07-02T02:44:10 *** GAit has quit IRC
342020-07-02T02:44:16 *** proofofkeags has joined #bitcoin-core-dev
352020-07-02T02:48:40 *** proofofkeags has quit IRC
362020-07-02T03:00:02 *** tummy has quit IRC
372020-07-02T03:04:00 *** proofofkeags has joined #bitcoin-core-dev
382020-07-02T03:12:44 *** troygiorshev has joined #bitcoin-core-dev
392020-07-02T03:19:19 *** proofofkeags has quit IRC
402020-07-02T03:31:12 *** proofofkeags has joined #bitcoin-core-dev
412020-07-02T03:31:12 *** proofofkeags has quit IRC
422020-07-02T03:31:24 *** proofofkeags has joined #bitcoin-core-dev
432020-07-02T03:31:31 *** proofofkeags has quit IRC
442020-07-02T03:34:59 *** EagleTM has joined #bitcoin-core-dev
452020-07-02T03:35:56 *** Eagle[TM] has quit IRC
462020-07-02T03:47:26 *** proofofkeags has joined #bitcoin-core-dev
472020-07-02T03:52:26 *** proofofkeags has quit IRC
482020-07-02T03:54:16 *** luke-jr has quit IRC
492020-07-02T03:55:25 *** Matt_P has joined #bitcoin-core-dev
502020-07-02T03:56:20 *** luke-jr has joined #bitcoin-core-dev
512020-07-02T04:08:39 *** shesek has joined #bitcoin-core-dev
522020-07-02T04:08:39 *** shesek has joined #bitcoin-core-dev
532020-07-02T04:39:12 *** bitdex has joined #bitcoin-core-dev
542020-07-02T04:46:08 *** bitdex has quit IRC
552020-07-02T04:58:43 *** vasild has quit IRC
562020-07-02T04:58:59 *** vasild has joined #bitcoin-core-dev
572020-07-02T05:00:55 *** troygiorshev has quit IRC
582020-07-02T05:01:58 *** vasild has quit IRC
592020-07-02T05:25:47 *** bitdex has joined #bitcoin-core-dev
602020-07-02T05:37:39 *** bitdex has quit IRC
612020-07-02T05:41:46 *** bitdex has joined #bitcoin-core-dev
622020-07-02T06:00:02 *** Matt_P has quit IRC
632020-07-02T06:09:03 *** alko has joined #bitcoin-core-dev
642020-07-02T06:09:22 *** EagleTM has quit IRC
652020-07-02T06:17:12 *** EagleTM has joined #bitcoin-core-dev
662020-07-02T06:22:02 *** aqu4 has joined #bitcoin-core-dev
672020-07-02T06:24:18 *** EagleTM has quit IRC
682020-07-02T06:25:06 *** marcoagner has joined #bitcoin-core-dev
692020-07-02T06:31:52 *** cfields has quit IRC
702020-07-02T06:33:00 *** cfields has joined #bitcoin-core-dev
712020-07-02T06:34:16 *** jonatack has quit IRC
722020-07-02T06:50:40 *** vasild has joined #bitcoin-core-dev
732020-07-02T06:58:17 *** Guyver2 has joined #bitcoin-core-dev
742020-07-02T07:25:49 *** afk11` has quit IRC
752020-07-02T07:26:15 *** afk11` has joined #bitcoin-core-dev
762020-07-02T07:57:05 *** kljasdfvv has quit IRC
772020-07-02T08:01:39 *** kljasdfvv has joined #bitcoin-core-dev
782020-07-02T08:29:03 *** AaronvanW has joined #bitcoin-core-dev
792020-07-02T08:30:29 *** go121212 has joined #bitcoin-core-dev
802020-07-02T08:32:47 *** go11111111111 has quit IRC
812020-07-02T09:00:02 *** aqu4 has quit IRC
822020-07-02T09:03:03 *** vasild has quit IRC
832020-07-02T09:19:59 *** vasild has joined #bitcoin-core-dev
842020-07-02T09:22:29 *** sledges has joined #bitcoin-core-dev
852020-07-02T09:44:08 *** dfmb_ has joined #bitcoin-core-dev
862020-07-02T09:48:53 *** promag has joined #bitcoin-core-dev
872020-07-02T09:51:29 *** dfmb_ has quit IRC
882020-07-02T10:02:48 *** isis_ is now known as isis
892020-07-02T10:03:28 *** Dino76Hettinger has joined #bitcoin-core-dev
902020-07-02T10:09:06 *** sdaftuar_ has quit IRC
912020-07-02T10:09:33 *** sdaftuar_ has joined #bitcoin-core-dev
922020-07-02T10:09:54 *** Pavlenex has joined #bitcoin-core-dev
932020-07-02T10:37:19 *** Dino76Hettinger has quit IRC
942020-07-02T10:42:09 *** promag has quit IRC
952020-07-02T10:51:58 *** reallll has joined #bitcoin-core-dev
962020-07-02T10:54:32 *** promag has joined #bitcoin-core-dev
972020-07-02T10:55:25 *** belcher has quit IRC
982020-07-02T11:26:51 *** bitcoin-git has joined #bitcoin-core-dev
992020-07-02T11:26:51 <bitcoin-git> [bitcoin] midnightmagic opened pull request #19430: build: configure.ac and netbsd-build.md for NetBSD (master...simple-fixes-for-netbsd-eventually-for-v0.20.0) https://github.com/bitcoin/bitcoin/pull/19430
1002020-07-02T11:26:52 *** bitcoin-git has left #bitcoin-core-dev
1012020-07-02T11:27:23 *** jonatack has joined #bitcoin-core-dev
1022020-07-02T11:33:23 *** vasild has quit IRC
1032020-07-02T11:36:01 *** vasild has joined #bitcoin-core-dev
1042020-07-02T11:36:19 *** promag has quit IRC
1052020-07-02T11:38:40 *** bitcoin-git has joined #bitcoin-core-dev
1062020-07-02T11:38:40 <bitcoin-git> [bitcoin] MarcoFalke opened pull request #19431: ci: Avoid failing pull requests destory the appveyor cache (master...2007-ciAppv) https://github.com/bitcoin/bitcoin/pull/19431
1072020-07-02T11:38:41 *** bitcoin-git has left #bitcoin-core-dev
1082020-07-02T11:48:42 *** promag has joined #bitcoin-core-dev
1092020-07-02T11:52:15 *** gleb1 has joined #bitcoin-core-dev
1102020-07-02T11:53:33 *** promag has quit IRC
1112020-07-02T11:54:34 *** gleb has quit IRC
1122020-07-02T11:54:35 *** gleb1 is now known as gleb
1132020-07-02T12:00:02 *** sledges has quit IRC
1142020-07-02T12:05:10 *** Pavlenex has quit IRC
1152020-07-02T12:17:39 *** Guyver2_ has joined #bitcoin-core-dev
1162020-07-02T12:20:50 *** Guyver2 has quit IRC
1172020-07-02T12:22:20 *** jesusabdullah has joined #bitcoin-core-dev
1182020-07-02T12:24:17 *** AaronvanW has quit IRC
1192020-07-02T12:27:13 *** Guyver2_ has quit IRC
1202020-07-02T12:31:13 *** ppisati has joined #bitcoin-core-dev
1212020-07-02T12:31:30 *** Highway61 has joined #bitcoin-core-dev
1222020-07-02T12:33:53 *** bitcoin-git has joined #bitcoin-core-dev
1232020-07-02T12:33:53 <bitcoin-git> [bitcoin] MarcoFalke closed pull request #19431: ci: Avoid failing pull requests destory the appveyor cache (master...2007-ciAppv) https://github.com/bitcoin/bitcoin/pull/19431
1242020-07-02T12:33:54 *** bitcoin-git has left #bitcoin-core-dev
1252020-07-02T12:34:13 *** bitcoin-git has joined #bitcoin-core-dev
1262020-07-02T12:34:13 <bitcoin-git> [bitcoin] MarcoFalke reopened pull request #19431: ci: Avoid failing pull requests destory the appveyor cache (master...2007-ciAppv) https://github.com/bitcoin/bitcoin/pull/19431
1272020-07-02T12:34:14 *** bitcoin-git has left #bitcoin-core-dev
1282020-07-02T12:34:50 *** shesek has quit IRC
1292020-07-02T12:38:20 *** IGHOR has quit IRC
1302020-07-02T12:42:04 *** IGHOR has joined #bitcoin-core-dev
1312020-07-02T12:46:06 *** ipriver has joined #bitcoin-core-dev
1322020-07-02T12:46:51 *** spaced0ut has joined #bitcoin-core-dev
1332020-07-02T12:47:11 *** promag has joined #bitcoin-core-dev
1342020-07-02T12:50:41 *** ipriver has quit IRC
1352020-07-02T12:52:46 *** reallll is now known as belcher
1362020-07-02T13:16:51 *** mdunnio has joined #bitcoin-core-dev
1372020-07-02T13:19:21 *** afk11` is now known as afk11
1382020-07-02T13:19:26 *** proofofkeags has joined #bitcoin-core-dev
1392020-07-02T13:34:58 *** Davterra has quit IRC
1402020-07-02T13:42:22 *** Davterra has joined #bitcoin-core-dev
1412020-07-02T13:47:12 *** troygiorshev has joined #bitcoin-core-dev
1422020-07-02T14:07:02 *** mdunnio has quit IRC
1432020-07-02T14:07:18 *** mdunnio has joined #bitcoin-core-dev
1442020-07-02T14:11:58 *** bitcoin-git has joined #bitcoin-core-dev
1452020-07-02T14:11:59 <bitcoin-git> [bitcoin] laanwj pushed 4 commits to master: https://github.com/bitcoin/bitcoin/compare/d6fe5b28dff4...7173a3c73ba9
1462020-07-02T14:12:00 <bitcoin-git> bitcoin/master fa2eb38 MarcoFalke: interfaces: Remove unused getDefaultChangeType
1472020-07-02T14:12:00 <bitcoin-git> bitcoin/master faddad7 MarcoFalke: Remove confusing OutputType::CHANGE_AUTO
1482020-07-02T14:12:00 <bitcoin-git> bitcoin/master fa927ff MarcoFalke: Enable Wswitch for OutputType
1492020-07-02T14:12:01 *** bitcoin-git has left #bitcoin-core-dev
1502020-07-02T14:12:19 *** bitcoin-git has joined #bitcoin-core-dev
1512020-07-02T14:12:19 <bitcoin-git> [bitcoin] laanwj merged pull request #19396: refactor: Remove confusing OutputType::CHANGE_AUTO (master...2006-WswitchOutputType) https://github.com/bitcoin/bitcoin/pull/19396
1522020-07-02T14:12:20 *** bitcoin-git has left #bitcoin-core-dev
1532020-07-02T14:15:09 *** Guyver2 has joined #bitcoin-core-dev
1542020-07-02T14:23:18 *** bitcoin-git has joined #bitcoin-core-dev
1552020-07-02T14:23:18 <bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/7173a3c73ba9...d77170d52687
1562020-07-02T14:23:19 <bitcoin-git> bitcoin/master fa12d8d MarcoFalke: ci: Add tsan suppression for race in wallet
1572020-07-02T14:23:19 <bitcoin-git> bitcoin/master d77170d MarcoFalke: Merge #19422: ci: Add tsan suppression for race in wallet
1582020-07-02T14:23:21 *** bitcoin-git has left #bitcoin-core-dev
1592020-07-02T14:23:38 *** bitcoin-git has joined #bitcoin-core-dev
1602020-07-02T14:23:38 <bitcoin-git> [bitcoin] MarcoFalke merged pull request #19422: ci: Add tsan suppression for race in wallet (master...2007-ciTsanSupW) https://github.com/bitcoin/bitcoin/pull/19422
1612020-07-02T14:23:39 *** bitcoin-git has left #bitcoin-core-dev
1622020-07-02T14:25:25 *** snifflin has joined #bitcoin-core-dev
1632020-07-02T14:28:27 *** bitcoin-git has joined #bitcoin-core-dev
1642020-07-02T14:28:27 <bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/d77170d52687...7027c67cac85
1652020-07-02T14:28:27 <bitcoin-git> bitcoin/master 870f0cd practicalswift: build: Add MemorySanitizer (MSan) in Travis to detect use of uninitialized...
1662020-07-02T14:28:28 <bitcoin-git> bitcoin/master 7027c67 MarcoFalke: Merge #18288: build: Add MemorySanitizer (MSan) in Travis to detect use of...
1672020-07-02T14:28:29 *** bitcoin-git has left #bitcoin-core-dev
1682020-07-02T14:29:27 *** bitcoin-git has joined #bitcoin-core-dev
1692020-07-02T14:29:27 <bitcoin-git> [bitcoin] MarcoFalke merged pull request #18288: build: Add MemorySanitizer (MSan) in Travis to detect use of uninitialized memory (master...msan-in-travis) https://github.com/bitcoin/bitcoin/pull/18288
1702020-07-02T14:29:29 *** bitcoin-git has left #bitcoin-core-dev
1712020-07-02T14:32:05 *** kristapsk has joined #bitcoin-core-dev
1722020-07-02T14:32:35 <jnewbery> aj: not that I know of. I just assumed that because of the comment "Maintain validation-specific state about nodes, protected by cs_main" there was some validation-specific state that needed to be guarded by cs_main. Maybe there isn't.
1732020-07-02T14:33:25 <aj> jnewbery: i took it as "it needs to be guarded by something, cs_main is something, ..."
1742020-07-02T14:33:33 *** GAit has joined #bitcoin-core-dev
1752020-07-02T14:33:45 *** bitdex has quit IRC
1762020-07-02T14:42:17 <snifflin> midnight ------->> [[ You are as pointless and irrelevant as an odious ignorant faulty heap of rancid donkey balls ]] <<-------
1772020-07-02T14:42:24 <snifflin> sorry for the interuption, carry on.
1782020-07-02T14:43:06 *** Relis has joined #bitcoin-core-dev
1792020-07-02T14:43:16 *** ChanServ sets mode: +o luke-jr
1802020-07-02T14:43:17 *** snifflin was kicked by luke-jr (User terminated!)
1812020-07-02T14:43:19 *** luke-jr sets mode: -o luke-jr
1822020-07-02T14:43:37 *** snifflin has joined #bitcoin-core-dev
1832020-07-02T14:43:57 *** snifflin has left #bitcoin-core-dev
1842020-07-02T14:49:42 *** alko has quit IRC
1852020-07-02T14:56:57 *** promag has quit IRC
1862020-07-02T15:00:01 *** jesusabdullah has quit IRC
1872020-07-02T15:01:16 *** promag has joined #bitcoin-core-dev
1882020-07-02T15:05:34 *** promag has quit IRC
1892020-07-02T15:08:51 *** proofofkeags has quit IRC
1902020-07-02T15:09:23 *** proofofkeags has joined #bitcoin-core-dev
1912020-07-02T15:10:23 *** go11111111111 has joined #bitcoin-core-dev
1922020-07-02T15:11:22 *** dongcarl2 has joined #bitcoin-core-dev
1932020-07-02T15:11:26 *** dongcarl2 has quit IRC
1942020-07-02T15:12:00 *** Victorsueca has joined #bitcoin-core-dev
1952020-07-02T15:12:11 *** niftynei_ has joined #bitcoin-core-dev
1962020-07-02T15:12:38 *** sharpiro_ has joined #bitcoin-core-dev
1972020-07-02T15:13:08 *** mol_ has joined #bitcoin-core-dev
1982020-07-02T15:13:48 *** ovovo has joined #bitcoin-core-dev
1992020-07-02T15:13:48 *** ovovo has joined #bitcoin-core-dev
2002020-07-02T15:13:50 *** proofofkeags has quit IRC
2012020-07-02T15:13:51 *** spaced0ut has quit IRC
2022020-07-02T15:14:15 *** owowo has quit IRC
2032020-07-02T15:14:15 *** niftynei has quit IRC
2042020-07-02T15:14:15 *** Victor_sueca has quit IRC
2052020-07-02T15:14:15 *** achow101 has quit IRC
2062020-07-02T15:14:18 *** TheRec_ has joined #bitcoin-core-dev
2072020-07-02T15:14:18 *** TheRec_ has joined #bitcoin-core-dev
2082020-07-02T15:14:25 *** achow101 has joined #bitcoin-core-dev
2092020-07-02T15:14:31 *** dongcarl has quit IRC
2102020-07-02T15:14:32 *** so has quit IRC
2112020-07-02T15:14:32 *** go121212 has quit IRC
2122020-07-02T15:14:32 *** TheRec has quit IRC
2132020-07-02T15:14:32 *** niska has quit IRC
2142020-07-02T15:14:32 *** niska has joined #bitcoin-core-dev
2152020-07-02T15:14:53 *** yancy has quit IRC
2162020-07-02T15:15:11 *** yancy has joined #bitcoin-core-dev
2172020-07-02T15:15:14 *** esotericnonsense has quit IRC
2182020-07-02T15:15:14 *** davec has quit IRC
2192020-07-02T15:15:22 *** a5m0 has quit IRC
2202020-07-02T15:15:22 *** sharpiro has quit IRC
2212020-07-02T15:15:22 *** TheHoliestRoger has quit IRC
2222020-07-02T15:15:44 *** so has joined #bitcoin-core-dev
2232020-07-02T15:15:50 *** mol has quit IRC
2242020-07-02T15:16:27 *** davec has joined #bitcoin-core-dev
2252020-07-02T15:17:00 *** a5m0 has joined #bitcoin-core-dev
2262020-07-02T15:17:51 *** esotericnonsense has joined #bitcoin-core-dev
2272020-07-02T15:17:59 *** TheHoliestRoger has joined #bitcoin-core-dev
2282020-07-02T15:19:40 *** benthecarman has joined #bitcoin-core-dev
2292020-07-02T15:20:37 *** dongcarl has joined #bitcoin-core-dev
2302020-07-02T15:22:11 *** fimp has joined #bitcoin-core-dev
2312020-07-02T15:22:36 *** fimp is now known as Guest36896
2322020-07-02T15:30:08 *** Relis has quit IRC
2332020-07-02T15:33:07 *** Pavlenex has joined #bitcoin-core-dev
2342020-07-02T15:33:49 *** spinza has quit IRC
2352020-07-02T15:35:10 *** promag has joined #bitcoin-core-dev
2362020-07-02T15:36:13 *** Pavlenex has quit IRC
2372020-07-02T15:38:21 *** Relis has joined #bitcoin-core-dev
2382020-07-02T15:47:46 *** Relis has quit IRC
2392020-07-02T15:48:07 *** spinza has joined #bitcoin-core-dev
2402020-07-02T15:48:13 *** benthecarman_ has joined #bitcoin-core-dev
2412020-07-02T15:50:54 *** benthecarman has quit IRC
2422020-07-02T15:54:42 *** benthecarman_ has quit IRC
2432020-07-02T15:58:30 *** Talkless has joined #bitcoin-core-dev
2442020-07-02T16:06:48 *** vasild has quit IRC
2452020-07-02T16:07:15 *** vasild has joined #bitcoin-core-dev
2462020-07-02T16:13:19 <jnewbery> aj: you're probably right. A lot has changed since that comment was written
2472020-07-02T16:13:46 <jnewbery> in which case, everything from CNodeState would eventually land in PeerState
2482020-07-02T16:14:27 <jnewbery> but even if not, having somewhere to put non-cs_main-guarded per-peer data in net processing is useful
2492020-07-02T16:17:13 <aj> jnewbery: i'd look at it more as "we're not 100% sure we can move everything in CNodeState out from under cs_main, so we'll do it piece by piece by moving parts of it to PeerState and making sure we understand each step", but same conclusion
2502020-07-02T16:17:32 <jnewbery> yes, agree 100%
2512020-07-02T16:18:01 <aj> jnewbery: having a shared_ptr and separate mutexes for each area struck me as odd, but i think i'm coming around to it
2522020-07-02T16:20:11 *** justanotheruser has quit IRC
2532020-07-02T16:20:41 <jnewbery> it's similar to how CNodes are currently managed, except we let the shared_ptrs do the refcounting instead of doing it ourselves
2542020-07-02T16:22:14 <sipa> for something like how CNodes are managed, using shared_ptrs instead makes sense i think
2552020-07-02T16:22:30 <sipa> i'm not convinced that's necessary or useful in net_processing though
2562020-07-02T16:23:32 <jnewbery> sipa: I was about to summon you because I know you have opinions about this
2572020-07-02T16:23:50 <sipa> (cnode has weird try_locks etc, and deletes node.objects while they're still in use etc)
2582020-07-02T16:24:00 <sipa> i think net_processing is fundamentally much simpler
2592020-07-02T16:24:54 <jnewbery> I think those try-locks can all be removed at this point. They were to protect against lock order inversions that can't happen now
2602020-07-02T16:24:58 <aj> i think if you don't use shared_ptr's then you need to retain a lock on the map while you use the value while you use it?
2612020-07-02T16:25:27 <sipa> i think all of net_processing can be one lock
2622020-07-02T16:26:23 <sipa> perhaps if you really want you can have one lock for block downloading, one for tx downloasing, one for addrman stuff, ...
2632020-07-02T16:26:32 <sipa> but i don't think that gains you much
2642020-07-02T16:26:55 <sipa> net_processing is inherently a serial affair of processing messages in some order
2652020-07-02T16:27:21 <aj> could process multiple peers simultaneously though?
2662020-07-02T16:27:29 <sipa> not really
2672020-07-02T16:27:38 <sipa> look at tx downloading
2682020-07-02T16:27:39 <jnewbery> aj: I want to at least leave that option open
2692020-07-02T16:28:28 <sipa> as soon as you hit any message related to tx downloading, you'd fall back to serial processing
2702020-07-02T16:28:40 <sipa> because there is global state that is shared across all peers
2712020-07-02T16:29:21 <sipa> for ping/pong you can do parallel processing, and maybe a few other small ones
2722020-07-02T16:30:17 <sipa> if net processing is really a bottleneck, i think we're better off just running multiple connmanns/net_processong instances, each with their own group of peers
2732020-07-02T16:30:25 <aj> jnewbery: (otoh, that option's always open: we just change the code later)
2742020-07-02T16:31:09 <aj> i wonder occasionally at whether we should have some reader/writer locks
2752020-07-02T16:31:11 <jnewbery> aj: breaking up global locks has proven to be difficult in the past. People have wanted to break up cs_main for as long as I've been on the project
2762020-07-02T16:31:58 <aj> jnewbery: i think having GUARDED_BY now might make that easier, presuming we annotate things from the start
2772020-07-02T16:32:23 <sipa> my ideal view is actually that net_processing has no locks; and is just a single "message processing" process that deals with incoming messages and creates outgoing one
2782020-07-02T16:32:43 <sipa> no locks for any internal data structures necessary, because only one thread can ever access them
2792020-07-02T16:33:04 <aj> you need to be able to access them via RPC for getpeerinfo and the like
2802020-07-02T16:33:31 <aj> but that would be super nice
2812020-07-02T16:33:50 <sipa> that could be done by having a separately-locked stats object that is updated by the net_processing thread
2822020-07-02T16:34:08 <jnewbery> sipa: I understand what you're saying, but I think moving the same locks from CNode to PeerState with perhaps some minor tidyup along the way is both the most conservative change and leaves the most options open for future changes
2832020-07-02T16:34:35 <sipa> jnewbery: i cannot comprehend why you want per-peer locks for thos
2842020-07-02T16:34:50 <jnewbery> if in the future we want to consolidate all those locks under cs_net_processing or whatever, that's fine
2852020-07-02T16:35:07 <sipa> for anything but the most basic messages, net_processing involves global state
2862020-07-02T16:35:44 <sipa> this is true for tx download, block download, compact block processing, ip addrs, ...
2872020-07-02T16:36:07 <jnewbery> sipa: because it allows us to make the changes slowly over time without having to completely change our locking model
2882020-07-02T16:36:08 *** justanotheruser has joined #bitcoin-core-dev
2892020-07-02T16:36:48 <sipa> jnewbery: sure, but you could do the same with a cs_tx_inv global lock, not a per-peer one
2902020-07-02T16:38:01 <sipa> amd similar things
2912020-07-02T16:38:49 <sipa> every piece of net_processing that has some (currently cs_main-locked) global state will need a replacement lock for that data structure
2922020-07-02T16:40:13 <jnewbery> how do you synchronize access to the PeerState list? take a lock and hold it for the entire time you're in net_processing?
2932020-07-02T16:41:15 <aj> map<nodeid, shared_ptr<PeerState>> GUARDED_BY(cs_peerstate), that only needs to be held while you access/update seems fine?
2942020-07-02T16:41:56 <aj> (with each member of PeerState protected by some global cs_whatever)
2952020-07-02T16:42:28 <jnewbery> if you're going to do that, then you need a refcount to make sure you don't delete it while someone else is holding the PeerState
2962020-07-02T16:42:40 <aj> shared_ptr does that?
2972020-07-02T16:42:50 <sipa> jnewbery: if all of net_processing is one lock, it's easy
2982020-07-02T16:43:04 <sipa> jnewbery: if you have separate sublocks... yeah, shared_ptr
2992020-07-02T16:44:47 <jnewbery> If we're doing separate subblocks (which I think we should for maximum flexibility) then there's very little difference between making those locks per-peer or global
3002020-07-02T16:45:08 <jnewbery> except if you do it per-peer, you leave the possibility open that you can do more parallelism in future
3012020-07-02T16:45:27 <jnewbery> if you make it a global it's very difficult to unpick that in a future change
3022020-07-02T16:46:09 <sipa> jnewbery: do you think my TxRequestTracker needs per-peer locks?
3032020-07-02T16:46:36 *** proofofkeags has joined #bitcoin-core-dev
3042020-07-02T16:46:52 <aj> having all the data be private to a net processing thread and not needing any locking seems a better future direection (and in line with moving p2p to its own process to reduce attack surface) to me
3052020-07-02T16:47:45 <jnewbery> sipa: no
3062020-07-02T16:48:06 <sipa> but perhaps before that PR, you'd be inclined to give it per-peer locks?
3072020-07-02T16:48:27 <jnewbery> sipa: probably not, no
3082020-07-02T16:48:29 <sipa> (for the per-peer fields)
3092020-07-02T16:48:52 <sipa> jnewbery: so why do you think it's useful for other things?
3102020-07-02T16:49:34 <sipa> apart from ping/pong, i don't think there is any net_processing that does not involve some direct or indirect global state
3112020-07-02T16:49:43 <jnewbery> I'm not arguing that having per-peer locks is the perfect way of doing things. It may not be. But it's the most obvious change for me that we bring the locks with us when we move the data from net to net_processing. And then maybe argue about what's the perfect way to do things.
3122020-07-02T16:49:46 <sipa> (including mempool, addrman, ...)
3132020-07-02T16:50:19 <sipa> jnewbery: i don't mind just moving things from net to net_processing, and taking the locks with it
3142020-07-02T16:50:21 *** nik-j has joined #bitcoin-core-dev
3152020-07-02T16:50:32 <jnewbery> there have been many attempts to split cs_main and none of them have gone anywhere because people have spent more energy arguing about what the perfect model is than making incremental improvements.
3162020-07-02T16:51:03 *** nik-j has quit IRC
3172020-07-02T16:51:05 <jnewbery> and we're in a position now where people actually add _more_ net processing data to net because it's easier to do that then fix the problem
3182020-07-02T16:51:21 <sipa> but if you're going to make changes to the locking system, i think that keeping/introducing per-peer locks is just needless complication
3192020-07-02T16:52:17 <jnewbery> sipa: have you reviewed: https://github.com/jnewbery/bitcoin/tree/2020-06-cs-main-split? I don't think it's too complicated.
3202020-07-02T16:52:54 <sipa> will do!
3212020-07-02T16:53:11 <jnewbery> and I think it's less reviewer effort to review something that reflects the existing locking rather than moving to a completely different synchronization system for net processing
3222020-07-02T16:53:24 <sipa> that makes sense
3232020-07-02T16:53:56 *** nik-j has joined #bitcoin-core-dev
3242020-07-02T16:54:07 <jnewbery> issue is here: https://github.com/bitcoin/bitcoin/issues/19398. I might try to summarize this conversation there at some point so it's not lost
3252020-07-02T16:54:12 <sipa> jnewbery: while i have you, regarding the global orphan processing... do you understand what i mean when saying that the current approach is observationally equivalent to immediately processing all dependent orphans recursively?
3262020-07-02T16:55:10 <jnewbery> from a single peer, it's observationally equivalent. I understand
3272020-07-02T16:55:42 <sipa> right ok
3282020-07-02T16:55:53 <sipa> so i don't think that's comparable
3292020-07-02T16:56:55 <sipa> it may be the case that we're ok with the behavior of making orphan processing observationally asynchronous... but that is a pretty fundamental change
3302020-07-02T16:57:28 <jnewbery> hmm I'm just struggling to see how fundamental that change is
3312020-07-02T16:57:41 <jnewbery> I have to go now, but would love to continue this conversation later
3322020-07-02T16:57:48 <sipa> and your PR presents it as: there is no good reasons why this is per-peer
3332020-07-02T16:58:05 <sipa> i think there is: it's to make orphan processing observationally synchronous
3342020-07-02T16:58:17 *** jonatack has quit IRC
3352020-07-02T16:58:50 <sipa> (and it took me a long time to realize that, i don't blame anyone for not realising that; it was badly documented too)
3362020-07-02T17:00:09 <sipa> maybe that reason is silly or unnecessary, but since our protocol is (mostly) synchronous, i think deviations from that need more than a "probably nothing relies on this" justification
3372020-07-02T17:02:02 <jnewbery> I think it's very different from getdata queueing, which should remain synchronous, and have more to say, but really must go now!
3382020-07-02T17:03:37 <sipa> note that i didn't nack your change
3392020-07-02T17:03:50 <sipa> i just think it needs more thought around potential impact
3402020-07-02T17:04:51 <sipa> jnewbery: sure, we'll discuss more later
3412020-07-02T17:08:29 *** molz_ has joined #bitcoin-core-dev
3422020-07-02T17:11:31 *** mol_ has quit IRC
3432020-07-02T17:13:56 *** nik-j_ has joined #bitcoin-core-dev
3442020-07-02T17:17:37 *** nik-j has quit IRC
3452020-07-02T17:19:05 *** nik-j_ has quit IRC
3462020-07-02T17:19:47 *** nik-j has joined #bitcoin-core-dev
3472020-07-02T17:21:06 *** promag has quit IRC
3482020-07-02T17:23:10 *** promag has joined #bitcoin-core-dev
3492020-07-02T17:23:16 *** ovovo has quit IRC
3502020-07-02T17:24:18 *** nik-j has quit IRC
3512020-07-02T17:24:23 *** owowo has joined #bitcoin-core-dev
3522020-07-02T17:26:19 <wumpus> promag: wasn't there still some linux distro that we support (for building on) that uses 2.0.x? I don't know tbh, I'm kind of tired of version bumping discussions :)
3532020-07-02T17:26:52 <promag> wumpus: I really don't know
3542020-07-02T17:27:20 <promag> hebasto: you were on this land too right?
3552020-07-02T17:27:43 <hebasto> wumpus: yes
3562020-07-02T17:27:46 <wumpus> sometimes it's like that's such a large part of the work done, do we need to bump dependency X or depency Y and to what version and what can we support and how can we support the (sometimes years old) linux distributiosn it needs to build on and blabla
3572020-07-02T17:28:03 <wumpus> kind of frustrating
3582020-07-02T17:28:27 <wumpus> (that doesn't mean unimportant though...)
3592020-07-02T17:28:50 <promag> but the argument is to support the libevent availble on the old distro package archive?
3602020-07-02T17:28:50 <hebasto> it seems I miss the context: 2.0.x of what?
3612020-07-02T17:28:56 <promag> libevent
3622020-07-02T17:28:58 <wumpus> libeventt
3632020-07-02T17:29:11 *** jonatack has joined #bitcoin-core-dev
3642020-07-02T17:30:52 <luke-jr> I wonder if we can automate version lookups
3652020-07-02T17:31:44 <hebasto> we should agree on supported distros list in advance, no?
3662020-07-02T17:32:00 <luke-jr> hebasto: so far it's basically been latest stable of major distros
3672020-07-02T17:32:07 <wumpus> luke-jr: that'd be nice
3682020-07-02T17:32:13 <promag> ideally we would bump to 2.2.0 - then we could drop a workaround
3692020-07-02T17:32:20 <wumpus> is that all?
3702020-07-02T17:32:31 <luke-jr> I suspect we could probably get most of this just by checking versions when RHEL releases
3712020-07-02T17:32:31 <promag> wumpus: ?
3722020-07-02T17:32:33 <wumpus> 'drop a workaround' is not very pressing to really break support for anything
3732020-07-02T17:32:39 <luke-jr> ^
3742020-07-02T17:32:47 <promag> wumpus: no
3752020-07-02T17:32:52 <wumpus> unless it's really a lot of code
3762020-07-02T17:33:00 <luke-jr> promag: even Gentoo doesn't have 2.2.0 yet
3772020-07-02T17:33:09 <promag> this #19420
3782020-07-02T17:33:11 <gribble> https://github.com/bitcoin/bitcoin/issues/19420 | http: Track active requests and wait for last to finish by promag · Pull Request #19420 · bitcoin/bitcoin · GitHub
3792020-07-02T17:33:21 <promag> luke-jr: and 2.1.*?
3802020-07-02T17:33:28 <wumpus> it's fine to conditionally support things for newer versions
3812020-07-02T17:33:32 <luke-jr> 2.1.x has been in Gentoo for a long time :P
3822020-07-02T17:33:34 <wumpus> it doesn't mean you always have to drop support for older ones
3832020-07-02T17:33:38 <luke-jr> 2.1.11 now
3842020-07-02T17:33:51 <wumpus> it's very good to say 'with this new version of libevent things will work better'
3852020-07-02T17:34:01 <wumpus> but unless it's a *critical* bug, breaking supporti s not worth it
3862020-07-02T17:34:11 <promag> I can do that
3872020-07-02T17:34:15 *** Evel-Knievel has joined #bitcoin-core-dev
3882020-07-02T17:34:36 <wumpus> for many practical RPC users, for ex, it's not a problem that say, libevent can't handle 1000 connnections per second
3892020-07-02T17:35:04 <wumpus> that it's in edge cases possible to exceed the number of file descriptors
3902020-07-02T17:35:15 <wumpus> yes, some people run into that, but by far most don't
3912020-07-02T17:35:18 <promag> the fix for that is not release I think
3922020-07-02T17:35:31 <wumpus> yea,it's an example :)
3932020-07-02T17:36:12 <promag> I also don't think it's really worth a bunch of #ifdef just to support some really old libevent
3942020-07-02T17:36:17 <wumpus> I just mean, in cases like that, you could add to the release notes 'please use version XXX of libevent or higher if you suffer from this issue'
3952020-07-02T17:37:06 <wumpus> that's very subjective, if it's a small amount of code that's only a little bit different
3962020-07-02T17:37:33 <promag> ok, I'll try to make the patch conditional
3972020-07-02T17:39:09 <wumpus> in any case if it passes travis it's probably ok, i think we now test the oldest versions that need to be supported in travis
3982020-07-02T17:39:34 <wumpus> and when C++17 requirement hits verious versions are going to be bumped anyhow
3992020-07-02T17:40:11 <wumpus> but if you have a PR open you're probably not going to want to wait for that
4002020-07-02T17:40:42 <promag> wumpus: it does passes on travis
4012020-07-02T17:41:50 <promag> right, we don't test what we say is the minimum supported version
4022020-07-02T17:42:04 *** Evel-Knievel has quit IRC
4032020-07-02T17:42:36 *** Evel-Knievel has joined #bitcoin-core-dev
4042020-07-02T17:49:37 *** jonatack has quit IRC
4052020-07-02T17:51:53 *** jonatack has joined #bitcoin-core-dev
4062020-07-02T17:55:12 <wumpus> hm then I'm wrong, I thought we had a run with minimum versions (some fedora version) now
4072020-07-02T17:55:25 *** alko89 has quit IRC
4082020-07-02T17:55:43 *** alko89 has joined #bitcoin-core-dev
4092020-07-02T17:56:29 *** filchef has joined #bitcoin-core-dev
4102020-07-02T18:00:02 *** Guest36896 has quit IRC
4112020-07-02T18:06:19 *** Talkless has quit IRC
4122020-07-02T18:07:37 *** Talkless has joined #bitcoin-core-dev
4132020-07-02T18:09:13 <promag> in doc/dependencies.md we say we support 2.0.21
4142020-07-02T18:09:48 <promag> but in #19420 I'm using 2.1 stuff
4152020-07-02T18:09:49 <gribble> https://github.com/bitcoin/bitcoin/issues/19420 | http: Track active requests and wait for last to finish by promag · Pull Request #19420 · bitcoin/bitcoin · GitHub
4162020-07-02T18:10:28 *** Talkless has joined #bitcoin-core-dev
4172020-07-02T18:21:27 *** corelax1 has joined #bitcoin-core-dev
4182020-07-02T18:25:15 *** Relis has joined #bitcoin-core-dev
4192020-07-02T18:26:18 *** alko has joined #bitcoin-core-dev
4202020-07-02T18:34:23 <jnewbery> sipa: the difference between queueing getdata requests and queueing orphan reconsideration is that orphan reconsideration won't ever result in a response message being sent to the peer that provided the parent
4212020-07-02T18:35:08 <jnewbery> (aside: it may result in the now-accepted orphan transaction being relayed to all peers, including the peer that provided the parent)
4222020-07-02T18:35:55 <jnewbery> so the only difference is whether our internal state (mempool, chaintip) has changed by the time we process any subsequent messages
4232020-07-02T18:37:30 <jnewbery> but that's already the case, since #15644 - the internal state may change because of messages received from other peers while the orphan reconsideration was queued
4242020-07-02T18:37:33 <gribble> https://github.com/bitcoin/bitcoin/issues/15644 | Make orphan processing interruptible by sipa · Pull Request #15644 · bitcoin/bitcoin · GitHub
4252020-07-02T18:38:18 <sipa> jnewbery: but that's not observable
4262020-07-02T18:38:39 <sipa> the only change introduced by 15644 is inserting pauses
4272020-07-02T18:38:58 <jnewbery> but the internal state can change during those pauses
4282020-07-02T18:39:09 <sipa> it's equivalent to having some line delay before receiving certain messages
4292020-07-02T18:40:49 <sipa> jnewbery: i think you don't understand what i mean by observation equivalence
4302020-07-02T18:41:11 <sipa> yes, of course it's operationally a change - the code is different, is does something else
4312020-07-02T18:41:17 <sipa> but peers can't tell that we do
4322020-07-02T18:41:17 *** Evel-Knievel has quit IRC
4332020-07-02T18:41:25 <sipa> (at least, individual peers can't)
4342020-07-02T18:41:27 *** Evel-Knievel has joined #bitcoin-core-dev
4352020-07-02T18:41:31 <jnewbery> the scenario you're concerned about, I think, is if a peer sends us TX1 followed by MSG2. We might process MSG2 before we've finished reconsidering the children of TX1
4362020-07-02T18:41:35 *** mdunnio has quit IRC
4372020-07-02T18:41:39 <sipa> indeed
4382020-07-02T18:41:43 *** alko has quit IRC
4392020-07-02T18:41:56 *** alko has joined #bitcoin-core-dev
4402020-07-02T18:42:19 <jnewbery> and the response to MSG2 might be different depending on our processing of the children of TX1
4412020-07-02T18:42:26 <sipa> one concern i had in particular was where MSG2 is a compact block, causing reconstruction to fail... but that's not the case because compact block reconconstruction also looks at the orphan pool
4422020-07-02T18:42:32 <sipa> but things like that is what i worry about
4432020-07-02T18:43:52 *** promag has quit IRC
4442020-07-02T18:45:42 <jnewbery> but it's also true that the response to MSG2 might be different if we'd received a message from peerB between accepting TX1 and reconsidering its children
4452020-07-02T18:46:28 <sipa> yes, obviously - but why is that relevant?
4462020-07-02T18:46:39 <sipa> there are no guarantees about speed of network messages
4472020-07-02T18:46:48 <sipa> or their ordering
4482020-07-02T18:46:58 <sipa> only that the messages from one peer arrive in the order they're sent
4492020-07-02T18:48:11 <jnewbery> because I don't see how that's any different from reconsidering the orphans on a global basis. There's no reason that the peer that provides the parent should care about orphans that another peer has sent, and there's no reason that processing should be coupled
4502020-07-02T18:48:36 <sipa> maybe
4512020-07-02T18:48:56 <sipa> it's just a fundamental change to suddenly not processing incoming messages in order anymore
4522020-07-02T18:49:29 <sipa> and i don't think just cleaner code is a justification for changing that
4532020-07-02T18:49:40 *** mdunnio has joined #bitcoin-core-dev
4542020-07-02T18:50:02 *** pinheadmz has quit IRC
4552020-07-02T18:50:52 <sipa> now, i cannot give an example for why this may matter to anyone
4562020-07-02T18:51:29 <jnewbery> If I have peerA and peerB, and construct TX1 -> TX2 -> ... -> TX100, then provide TX2-TX100 to peerB, and provide TX1 to peerA
4572020-07-02T18:51:49 *** Evel-Knievel has quit IRC
4582020-07-02T18:52:19 <jnewbery> peerA relays TX1 to peerB, and peerB then reconsiders TX2-TX100 in the context of ReceiveMessages(peerA)
4592020-07-02T18:52:28 *** Evel-Knievel has joined #bitcoin-core-dev
4602020-07-02T18:53:09 <sipa> are you saying that the current behavior is a privacy leak?
4612020-07-02T18:53:30 <sipa> if so, that'd be a better justification
4622020-07-02T18:53:39 <jnewbery> No, I just don't see how it makes any sense
4632020-07-02T18:53:56 <jnewbery> ok, I'll try to come up with a privacy leak for post-justification :)
4642020-07-02T18:54:46 <sipa> if compact blocks didn't consider reconstruction from recent orphans, i think that'd be a very good example
4652020-07-02T18:54:51 <sipa> it isn't - but it's very close
4662020-07-02T18:55:43 <jnewbery> if at the same time, I provided TX2' to both peerA and peerB, then peerA would relay TX2' to peerB
4672020-07-02T18:56:37 *** pinheadmz has joined #bitcoin-core-dev
4682020-07-02T18:57:04 <jnewbery> prior to 15644, TX2 would always win, because it's reprocessed in the same ReceiveMessages() call
4692020-07-02T18:57:33 <jnewbery> since 15644, TX2' might win, because it could be processed in one of those pauses
4702020-07-02T18:58:20 <jnewbery> which is observable by peerA because peerB doesn't getdata(TX2')
4712020-07-02T19:00:37 <wumpus> #startmeeting
4722020-07-02T19:00:37 <lightningbot> Meeting started Thu Jul 2 19:00:37 2020 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
4732020-07-02T19:00:37 <lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
4742020-07-02T19:00:40 *** gzhao408 has joined #bitcoin-core-dev
4752020-07-02T19:00:42 <achow101> hi
4762020-07-02T19:00:45 <jnewbery> hi
4772020-07-02T19:00:45 <hebasto> hi
4782020-07-02T19:00:45 <jkczyz> hi
4792020-07-02T19:00:48 <jonasschnelli> Hi
4802020-07-02T19:00:56 <wumpus> #bitcoin-core-dev Meeting: wumpus sipa gmaxwell jonasschnelli morcos luke-jr sdaftuar jtimon cfields petertodd kanzure bluematt instagibbs phantomcircuit codeshark michagogo marcofalke paveljanik NicolasDorier jl2012 achow101 meshcollider jnewbery maaku fanquake promag provoostenator aj Chris_Stewart_5 dongcarl gwillen jamesob ken281221 ryanofsky gleb moneyball kvaciral ariard digi_james amiti fjahr
4812020-07-02T19:00:58 <wumpus> jeremyrubin lightlike emilengler jonatack hebasto jb55 elichai2
4822020-07-02T19:01:06 <kanzure> hi
4832020-07-02T19:01:15 <provoostenator> hi
4842020-07-02T19:01:17 <meshcollider> hi
4852020-07-02T19:01:19 <jamesob> hi
4862020-07-02T19:01:25 <aj> hi
4872020-07-02T19:01:28 <fjahr> hi
4882020-07-02T19:01:35 <pinheadmz> hi
4892020-07-02T19:01:59 <jeremyrubin> hi
4902020-07-02T19:02:07 <wumpus> looks like there have been no proposed meeting topics this week in http://gnusha.org/bitcoin-core-dev/proposedmeetingtopics.txt
4912020-07-02T19:02:29 <ariard> hi
4922020-07-02T19:02:54 <dongcarl> hi
4932020-07-02T19:02:58 <gzhao408> hi
4942020-07-02T19:02:59 <elichai2> Hi
4952020-07-02T19:03:43 <wumpus> any last minute topic proposals?
4962020-07-02T19:04:04 <dongcarl> would like to discuss the various places we put documentation a bit, but perhaps after the meeting
4972020-07-02T19:04:47 <wumpus> ok yes that's a bit vague, either you want to prpose it as a meeting topic or not?
4982020-07-02T19:05:27 <wumpus> #topic High priority for review
4992020-07-02T19:05:28 <dongcarl> sorry, I should be more clear, no, let's table that till after meeting
5002020-07-02T19:05:38 <achow101> #19324 for hi prio
5012020-07-02T19:05:42 <gribble> https://github.com/bitcoin/bitcoin/issues/19324 | wallet: Move BerkeleyBatch static functions to BerkeleyDatabase by achow101 · Pull Request #19324 · bitcoin/bitcoin · GitHub
5022020-07-02T19:05:52 <wumpus> https://github.com/bitcoin/bitcoin/projects/8 11 blockers, 1 bugfix, 3 chasing concept ACK pending
5032020-07-02T19:05:54 <wumpus> dongcarl: ok!
5042020-07-02T19:06:10 <wumpus> achow101: yup. I predicted that one :)
5052020-07-02T19:06:57 <wumpus> added
5062020-07-02T19:06:59 <achow101> heh
5072020-07-02T19:07:43 <dongcarl> #17919 please
5082020-07-02T19:07:45 <gribble> https://github.com/bitcoin/bitcoin/issues/17919 | [DO NOT MERGE] depends: Allow building with system clang by dongcarl · Pull Request #17919 · bitcoin/bitcoin · GitHub
5092020-07-02T19:08:04 *** Talkless has quit IRC
5102020-07-02T19:08:08 <dongcarl> (the DO NOT MERGE is because of the last commit which is a demo, can be dropped easily)
5112020-07-02T19:08:20 <wumpus> DO NOT MERGE ;)
5122020-07-02T19:08:39 <wumpus> dongcarl: good to know!
5132020-07-02T19:09:04 <wumpus> added
5142020-07-02T19:09:05 <jeremyrubin> #proposedmeetingtopic I'd like to see the nanobench stuff get finished
5152020-07-02T19:10:38 <wumpus> anything in high priority for review that is mergable or close to being mergable?
5162020-07-02T19:10:49 *** Davterra has quit IRC
5172020-07-02T19:11:02 <wumpus> we've merged a few smaller ones this week
5182020-07-02T19:11:29 *** Davterra has joined #bitcoin-core-dev
5192020-07-02T19:11:34 <wumpus> but looks like most are still busy in mid-review
5202020-07-02T19:12:22 <jeremyrubin> I think #18191 is mergeable.
5212020-07-02T19:12:24 <wumpus> #18637 looks somewhat nearing readyness
5222020-07-02T19:12:24 <gribble> https://github.com/bitcoin/bitcoin/issues/18191 | Change UpdateForDescendants to use Epochs by JeremyRubin · Pull Request #18191 · bitcoin/bitcoin · GitHub
5232020-07-02T19:12:27 <gribble> https://github.com/bitcoin/bitcoin/issues/18637 | coins: allow cache resize after init by jamesob · Pull Request #18637 · bitcoin/bitcoin · GitHub
5242020-07-02T19:13:04 <jamesob> I'm +1 :)
5252020-07-02T19:14:04 <wumpus> jeremyrubin: ok will take a look after the meeting
5262020-07-02T19:14:29 <jnewbery> jeremyrubin: I can't see how 18191 is mergeable. I'd expect more than one ACK for changes to the mempool
5272020-07-02T19:15:12 <wumpus> #topic Nanobench (jeremyrubin)
5282020-07-02T19:15:15 <wumpus> jnewbery: yes
5292020-07-02T19:15:53 <wumpus> seems a bit premature
5302020-07-02T19:16:42 <jeremyrubin> fair
5312020-07-02T19:17:39 <jeremyrubin> I had thought some of the other reviewers had correctness acked the code, but there is disagreement about benchmarking. I don't think it requires further benching at this time, as it's primarily a memory improvement and has some benches. But can always do more.
5322020-07-02T19:17:53 <wumpus> I also wonder what sdaftuar_ thinks about it now, initially he didn't think it'd be worth the increase of complexity
5332020-07-02T19:18:14 *** owowo has quit IRC
5342020-07-02T19:19:10 <ariard> I'll try to review 18191 before next meeting, has been on my list for a while
5352020-07-02T19:19:28 <wumpus> ariard: thanks!
5362020-07-02T19:19:32 <jeremyrubin> Can send you some more details privately
5372020-07-02T19:19:44 <jeremyrubin> it's a non trivial memory improvement for reorgs
5382020-07-02T19:20:14 <jeremyrubin> ok onto nanobench
5392020-07-02T19:20:21 <jeremyrubin> I think it's in pretty good shape
5402020-07-02T19:20:55 <jeremyrubin> MarcoFalke needs to run with frequency lock enabled?
5412020-07-02T19:20:55 <dongcarl> jeremyrubin: can you link to prs?
5422020-07-02T19:20:58 <jeremyrubin> Uhh
5432020-07-02T19:21:01 *** nik-j has joined #bitcoin-core-dev
5442020-07-02T19:21:18 <jeremyrubin> #18011
5452020-07-02T19:21:21 <gribble> https://github.com/bitcoin/bitcoin/issues/18011 | Replace current benchmarking framework with nanobench by martinus · Pull Request #18011 · bitcoin/bitcoin · GitHub
5462020-07-02T19:22:51 <wumpus> I have no opinion on replacing the benchmark framework, will leave it to people more involved with benchmarking
5472020-07-02T19:23:41 *** Evel-Knievel has quit IRC
5482020-07-02T19:23:49 <jeremyrubin> I think we can just ship it and see what/if anything breaks. AFAIK it has been made output compatible with prior benching output format
5492020-07-02T19:23:51 *** Evel-Knievel has joined #bitcoin-core-dev
5502020-07-02T19:24:01 <jeremyrubin> the only concrete harm is that the relative numbers won't be comparable anymore
5512020-07-02T19:24:18 <wumpus> so this doesn't add any dependencies?
5522020-07-02T19:24:30 <jeremyrubin> It adds a checked in header file
5532020-07-02T19:24:39 <jeremyrubin> Which martinus, the comitter, is the maintainer of
5542020-07-02T19:24:45 <wumpus> yeah that's okay
5552020-07-02T19:25:04 *** owowo has joined #bitcoin-core-dev
5562020-07-02T19:25:09 <jeremyrubin> So I'd just be happy to have someone who actively is adding benching features
5572020-07-02T19:25:22 *** nik-j has quit IRC
5582020-07-02T19:25:27 <jeremyrubin> It has some nice new bells and whistles around helping auto categorize big o
5592020-07-02T19:25:31 <wumpus> that's a point
5602020-07-02T19:25:37 <sipa> jeremyrubin: why won't relative numbers be conparable?
5612020-07-02T19:26:03 <jeremyrubin> It does a better job I think of auto detecting how many runs are required or something
5622020-07-02T19:26:07 <jeremyrubin> so we do fewer runs
5632020-07-02T19:26:20 <jeremyrubin> and there is less measurment-in-benchmark overhead I think?
5642020-07-02T19:26:23 <sipa> ok, but the benchmark numbers should be the same?
5652020-07-02T19:26:31 <sipa> or at least, closer to reality
5662020-07-02T19:26:42 <dongcarl> "measure instructions, cycles, branches, instructions per cycle, branch misses" <- is this not done currently (w/o nanobench)?
5672020-07-02T19:26:48 <jeremyrubin> Well the hot-loop code I think is faster? But the benches themselves won't change
5682020-07-02T19:26:55 <jeremyrubin> No it is not dongcarl
5692020-07-02T19:27:07 <jeremyrubin> So we get a bunch of new and cool outputs
5702020-07-02T19:27:31 <jeremyrubin> But also a compatible output mode for tooling that can't be updated
5712020-07-02T19:27:34 <sipa> current master just tests min/max/avg time of some fixed number of iterations of a test
5722020-07-02T19:27:46 <wumpus> adding outputs is great
5732020-07-02T19:27:56 <dongcarl> that sounds like a worthy feature to me
5742020-07-02T19:28:13 <wumpus> does this still work on non-x86?
5752020-07-02T19:29:04 <wumpus> I added measuring instruction cycles at some point but this was also removed again
5762020-07-02T19:29:17 <jeremyrubin> Not sure wumpus
5772020-07-02T19:29:39 <dongcarl> I don't see anything in their CI for non-x86
5782020-07-02T19:29:53 <wumpus> jeremyrubin: I don't mind if it doesn't report all the numbers on non-x86, just that it compiles/works
5792020-07-02T19:30:13 <sipa> that seems reasonable
5802020-07-02T19:30:30 *** sipsorcery has joined #bitcoin-core-dev
5812020-07-02T19:30:58 <jeremyrubin> I don'
5822020-07-02T19:31:15 <jeremyrubin> *don't see anything explicitly incompatible except the turbo detection?
5832020-07-02T19:31:17 <wumpus> the PR currently doesn't pass travis, not that that says everything :)
5842020-07-02T19:31:23 <jeremyrubin> Maybe I'm missing some syscall
5852020-07-02T19:31:59 <sipa> well how does it count instructions?
5862020-07-02T19:32:15 <sipa> because on ARM only privileged code can do that
5872020-07-02T19:32:29 <sipa> s/instructions/clock ticks/
5882020-07-02T19:32:32 <jeremyrubin> Failures on Travis are build systme related
5892020-07-02T19:32:43 <jeremyrubin> not inherent
5902020-07-02T19:32:53 <jeremyrubin> https://travis-ci.org/github/bitcoin/bitcoin/builds/697933849
5912020-07-02T19:33:07 <sipa> ok let's try to give it some.renewed review attention?
5922020-07-02T19:33:32 <sipa> seems useful to me to make progress on (i think the current benchmark system is pretty stupid...)
5932020-07-02T19:33:38 <wumpus> the ARM failure is really srange, just some timeout on package download
5942020-07-02T19:33:47 <wumpus> restarting those jobs
5952020-07-02T19:33:49 <dongcarl> classic Travis
5962020-07-02T19:34:01 <wumpus> dongcarl: yes :)
5972020-07-02T19:34:03 <dongcarl> *90's sitcom noises*
5982020-07-02T19:34:41 <jeremyrubin> martinus claims that it only does performance counters where available
5992020-07-02T19:34:44 <wumpus> I'm naive enough to think when ARM tests fails to think it's some real problem with non-x86 architectures
6002020-07-02T19:34:49 <jeremyrubin> So my guess is it's auto detected
6012020-07-02T19:34:58 *** AaronvanW has joined #bitcoin-core-dev
6022020-07-02T19:34:59 <wumpus> anyhow, yes, let's continue review on it
6032020-07-02T19:35:26 <wumpus> any other topics?
6042020-07-02T19:37:40 *** Evel-Knievel has quit IRC
6052020-07-02T19:37:48 * wumpus wonders why #18011 is in mempool improvements
6062020-07-02T19:37:50 <gribble> https://github.com/bitcoin/bitcoin/issues/18011 | Replace current benchmarking framework with nanobench by martinus · Pull Request #18011 · bitcoin/bitcoin · GitHub
6072020-07-02T19:38:14 <wumpus> let's add it to high prio for review as well at least
6082020-07-02T19:38:37 *** Evel-Knievel has joined #bitcoin-core-dev
6092020-07-02T19:39:25 <dongcarl> Should the "need conceptual review" tag be removed or no?
6102020-07-02T19:40:13 <wumpus> dongcarl: do you think so?
6112020-07-02T19:40:40 <dongcarl> I haven't heard oppositions to the concept, just code kinks we might need to work out?
6122020-07-02T19:41:04 <wumpus> oh, you mean removing it from that particular PR, not in general, sorry, I agree :)
6132020-07-02T19:41:34 <wumpus> done
6142020-07-02T19:41:40 <dongcarl> :)
6152020-07-02T19:42:06 <wumpus> #endmeeting
6162020-07-02T19:42:06 <lightningbot> Meeting ended Thu Jul 2 19:42:06 2020 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
6172020-07-02T19:42:06 <lightningbot> Minutes: http://www.erisian.com.au/meetbot/bitcoin-core-dev/2020/bitcoin-core-dev.2020-07-02-19.00.html
6182020-07-02T19:42:06 <lightningbot> Minutes (text): http://www.erisian.com.au/meetbot/bitcoin-core-dev/2020/bitcoin-core-dev.2020-07-02-19.00.txt
6192020-07-02T19:42:06 <lightningbot> Log: http://www.erisian.com.au/meetbot/bitcoin-core-dev/2020/bitcoin-core-dev.2020-07-02-19.00.log.html
6202020-07-02T19:42:10 *** shesek has joined #bitcoin-core-dev
6212020-07-02T19:44:20 <jnewbery> sipa: is the scenario I describe above interesting/relevant? It does seem like there can be observable differences for peers before/after 15644, or maybe I really don't understand what observable equivalence is
6222020-07-02T19:45:14 <sipa> jnewbery: yeah, i believe there are multiple-peer scenarios where overall behavior is differemt before/after 15644
6232020-07-02T19:45:47 <sipa> i don't think it changes the discussion though
6242020-07-02T19:46:28 <sipa> 15644 was an intentional change in behavior, which tries to maintain protocol guarantees as much as possible
6252020-07-02T19:46:29 *** shesek has quit IRC
6262020-07-02T19:47:16 <sipa> i don't like introducing p2p behavior for the sake of simplifying code
6272020-07-02T19:47:20 <sipa> *changes
6282020-07-02T19:47:44 <sipa> improving privacy, dos resistance, performance, ... are very different justifications
6292020-07-02T19:47:54 <wumpus> not as only rationale
6302020-07-02T19:47:58 <jeremyrubin> It's in mempool improvements because I introduced a framework for measuring asymptotics, to help with concerns that I didn't have benchmarks showing the refactors are stronger.
6312020-07-02T19:48:11 <jeremyrubin> But my asymptote feature was rejected
6322020-07-02T19:48:26 <jeremyrubin> And martinus introduced a separate new framework which solved my problem
6332020-07-02T19:48:33 <jeremyrubin> So I dropped my patches in favor of his
6342020-07-02T19:48:38 <jeremyrubin> And added it to the mempool project
6352020-07-02T19:48:52 <wumpus> jeremyrubin: makes sense
6362020-07-02T19:49:19 <jeremyrubin> And this all relates back to fixing a bunch of mempool problems people have, like txn pinning :)
6372020-07-02T19:49:54 *** Pavlenex has joined #bitcoin-core-dev
6382020-07-02T19:50:00 *** molz_ has quit IRC
6392020-07-02T19:50:34 <wumpus> so the improvements in the benchmark framework are mostly motivated from increasing mempool performance, i think that's fair,that's one of the things where performance matters (besides initial sync validation)
6402020-07-02T19:51:58 <jnewbery> sipa: I'm just struggling to see how 'processing a tx that another peer provided before processing you next message' is a protocol guarantee. It seems more a quirk of the existing implementation
6412020-07-02T19:52:34 *** nik-j has joined #bitcoin-core-dev
6422020-07-02T19:52:41 <sipa> jnewbery: i think processing messages in order, except in a few well-defined cases, is a protocol guarantee
6432020-07-02T19:53:13 <sipa> and with a bunch of more thinking i can probably be convinced that it doesn't impact anything significant
6442020-07-02T19:53:24 <sipa> but is that worth it, compared to just keeping the code?
6452020-07-02T19:53:39 <jeremyrubin> jnewbery: btw my bad on ACK counting. Suhas had ACK'd the immediate predecessor to that PR, which still contained a bug, which the follow on had addressed. But he was yet to re-ack the follow on cleanups, which certainly should see more ACKs before merge.
6462020-07-02T19:54:22 <jnewbery> yes, processing messages _from the same peer_ in order is the guarantee. That's not what's changing here
6472020-07-02T19:55:16 <sipa> jnewbery: yes it is? there used to be a guarantee that if a peer sends a message, all successor messages are treated having fully processed that tx
6482020-07-02T19:55:23 <sipa> that's no longer the case
6492020-07-02T19:55:54 <sipa> perhaps you don't think processing descendants should be treated as part of processing a tx
6502020-07-02T19:56:00 <jnewbery> there are no guarantees at all about the orphan pool
6512020-07-02T19:56:09 <jnewbery> sipa: indeed I don't!
6522020-07-02T19:56:10 <sipa> there used to be!
6532020-07-02T19:57:02 <jnewbery> the orphan pool is unauthenticated data provided by any peer, that's limited by random eviction. How can there be any guarantees about it?
6542020-07-02T19:57:22 <jnewbery> *unvalidated
6552020-07-02T19:57:28 <sipa> that's fair
6562020-07-02T19:58:12 <sipa> though right now, if i give you two dependent transactions, in a short timeframe, out of order, afterwards you will either have accepted them, or you're never going to
6572020-07-02T19:58:46 <sipa> this changes that to be delayed to some arbitrary amount of time in the future
6582020-07-02T19:59:45 <sipa> for no reason, i think
6592020-07-02T19:59:53 *** mol has joined #bitcoin-core-dev
6602020-07-02T20:03:24 *** AaronvanW has quit IRC
6612020-07-02T20:04:20 *** alko has quit IRC
6622020-07-02T20:05:06 <jnewbery> this seems true, although I can't think of why it'd be important. 'arbitrary amount of time in future' is the order of milliseconds.
6632020-07-02T20:07:05 <sipa> it could be minutes if there is a block processed in between
6642020-07-02T20:07:16 <sipa> and you need to load utxos from a slow disk
6652020-07-02T20:07:36 <sipa> but i agree - which is why i say i may be convinced there is no risk
6662020-07-02T20:07:50 <sipa> i just don't think it's worth my time or others' to figure this out
6672020-07-02T20:07:58 <sipa> if the only justification is simpler code
6682020-07-02T20:10:08 <jnewbery> The marginal benefit to you of simpler code is much smaller than the marginal benefit to other people of simpler code (since you wrote most of the existing code). Perhaps that's why our assessments of this are different
6692020-07-02T20:10:41 <jnewbery> I've been working on the code for 4 years, and the split between net and net_processing still barely makes sense to me
6702020-07-02T20:10:56 <sipa> sure
6712020-07-02T20:11:04 <sipa> i'm all on favor of fixing that split!
6722020-07-02T20:11:11 <sipa> but what does this have to do with that
6732020-07-02T20:11:28 <jnewbery> moving orphan processing data from net to net processing
6742020-07-02T20:11:40 <sipa> great
6752020-07-02T20:11:53 <sipa> why does that need a global for this queue instead of a per peer structure?
6762020-07-02T20:12:06 <jnewbery> because per-peer is protected by cs_main
6772020-07-02T20:12:36 <sipa> and in the future it will be protected by some net_processing lock?
6782020-07-02T20:12:45 <sipa> is that what is motivating this?
6792020-07-02T20:14:20 <jnewbery> yes
6802020-07-02T20:14:38 <jnewbery> removing application-layer data from CNode in net
6812020-07-02T20:15:15 <sipa> you can move it from CNode to CNodeState, no?
6822020-07-02T20:15:21 <sipa> without changing it to a global
6832020-07-02T20:15:43 <sipa> it absolutely does not belong in CNode
6842020-07-02T20:16:02 <jnewbery> I'd also like orphan tracking to be a separate global module, just like txrelay will be
6852020-07-02T20:16:26 <sipa> sounds great
6862020-07-02T20:16:34 <jnewbery> why did you put it in CNode?
6872020-07-02T20:16:43 <sipa> because it was easier
6882020-07-02T20:16:51 <sipa> i think
6892020-07-02T20:17:23 <sipa> but it being a global module doesn't prevent it from having per-peer data
6902020-07-02T20:19:54 <jnewbery> I can move it to CNodeState, it just seems a shame that a project which aims to reduce cs_main locking adds yet more data under that mutex
6912020-07-02T20:20:19 <sipa> it's a shame that net_processing's globals are locked by cs_main
6922020-07-02T20:20:31 <dongcarl> jnewbery: would it make sense in PeerState?
6932020-07-02T20:20:39 <sipa> there is no solution to that except first moving everything to its place, and then separating the locks
6942020-07-02T20:20:54 <sipa> i don't think moving things to their correct place is a step backward
6952020-07-02T20:21:02 *** gzhao408 has quit IRC
6962020-07-02T20:21:05 <jnewbery> dongcarl: yes, but we also disagree about how to implement PeerState
6972020-07-02T20:21:08 <sipa> dongcarl: PeerState and CNodeState should eventually be one thing
6982020-07-02T20:22:03 *** Pavlenex has quit IRC
6992020-07-02T20:22:12 <dongcarl> What is the disagreement about PeerState?
7002020-07-02T20:22:23 <sipa> no per-peer locks in net_processing'
7012020-07-02T20:22:29 <sipa> (in my opinion)
7022020-07-02T20:22:55 <jnewbery> whether we should have One Big Lock for net processing, or whether each peer should get its own lock. Scrollback to ~12:30
7032020-07-02T20:23:24 <dongcarl> okay cool, will scroll
7042020-07-02T20:23:32 <aj> a few big locks was still on the table i thought
7052020-07-02T20:23:35 <sipa> very short summary: net_processing for almost everything has inherent cross-peer state for coordination; per peer locks don't mae sense if everything will end up grabbing global locks anyway
7062020-07-02T20:24:04 <sipa> (of my opinion; i don't want to misrepresent anyone else's)
7072020-07-02T20:24:07 <jnewbery> sipa: that's a short summary of your side!
7082020-07-02T20:24:29 <sipa> yes, i clarified :)
7092020-07-02T20:25:08 <sipa> jnewbery: you seem to somehow think that having smaller-scoped locks is inherently better than big ones
7102020-07-02T20:25:11 <jnewbery> short summary of mine: we have per-peer locking of these concepts already in net. Keeping those locks when you move the data to net processing is the simplest to review change, and would easily allow consolidating them later if that's what's desired.
7112020-07-02T20:25:34 <sipa> jnewbery: i have no problem with keeping the locks while just moving things
7122020-07-02T20:25:57 <dongcarl> I don't see a disagreement about the end goal
7132020-07-02T20:26:04 <jnewbery> sipa: phew, we agree on something :)
7142020-07-02T20:26:12 <dongcarl> Is it just the intermediate steps that is different?
7152020-07-02T20:26:17 <sipa> i do have a problem it being used as motivation to introduce semantics changes, which wouldn't be needed for the (in my view) ideal end scenario
7162020-07-02T20:26:39 <sipa> and i see no problem with just moving orphan_work_set as a per-peer structure, protected by cs_main, to CNodeState
7172020-07-02T20:26:51 <sipa> and then later fixing the locks, without it ever becoming a global
7182020-07-02T20:34:57 *** Evel-Knievel has quit IRC
7192020-07-02T20:35:46 <sipa> i think per-peer data structures for net_processing things are a silly artefact that results from them historically having been stored in CNode, where there was no clear global lock to protect them
7202020-07-02T20:39:24 <dongcarl> sipa: I'm confused, where would per-peer application-level data go if there were no per-peer data structures in net_processing?
7212020-07-02T20:39:40 <sipa> dongcarl: per-peer data structures, absolutely!
7222020-07-02T20:39:44 <sipa> no per-peer *locks*
7232020-07-02T20:40:01 <sipa> because most things inherently need to access global coordination anyway
7242020-07-02T20:40:12 *** Evel-Knievel has joined #bitcoin-core-dev
7252020-07-02T20:41:28 <sipa> for example, g_already_asked_for is a global coordination structure, and there isn't much that can be done to the per-peer m_tx_process_time, m_tx_announced, m_tx_in_flight without also grabbing the lock that protects g_already_asked for... so there is no point in having per-peer locks for those
7262020-07-02T20:41:58 <sipa> oh i see, i missed "locks" in my sentence above
7272020-07-02T20:44:19 *** Guyver2 has quit IRC
7282020-07-02T20:45:02 <dongcarl> Hmm I think I need to read more code. Will grep for those
7292020-07-02T20:45:16 <jnewbery> dongcarl: they don't exist yet
7302020-07-02T20:45:28 <sipa> ?
7312020-07-02T20:45:30 <jnewbery> currently all per-peer data in net processing is protected by cs_main
7322020-07-02T20:46:06 <jnewbery> there are more granular locks for data in CNode. I'm suggesting those move into PeerState along with the data they're protecting
7332020-07-02T20:46:28 <sipa> dongcarl: don't familiarize yourself with them too much; they're all going away in #19184
7342020-07-02T20:46:31 <gribble> https://github.com/bitcoin/bitcoin/issues/19184 | Overhaul transaction request logic by sipa · Pull Request #19184 · bitcoin/bitcoin · GitHub
7352020-07-02T20:46:31 <jnewbery> sipa is saying that's an unnecessary complication because he'd like to get rid of them
7362020-07-02T20:46:51 <dongcarl> he'd like to get rid of the granular locks?
7372020-07-02T20:46:55 <sipa> yes
7382020-07-02T20:46:56 <jnewbery> yes
7392020-07-02T20:46:59 <sipa> they're stupid
7402020-07-02T20:47:10 <dongcarl> Why not do john's first then get rid of the granular locks?
7412020-07-02T20:47:18 <sipa> that's what i'm suggesting!
7422020-07-02T20:47:25 <sipa> move them
7432020-07-02T20:47:36 <sipa> and then get rid of them in a move to net_processing globals
7442020-07-02T20:48:17 <sipa> but jnewbery seems to think that moving orphan_work_set to net_processing while keeping it under cs_main is sad
7452020-07-02T20:48:48 <jnewbery> I don't see much benefit in removing the granular locks later, but we can save that for another day
7462020-07-02T20:48:55 <sipa> :'(
7472020-07-02T20:49:13 <jnewbery> jnewbery does think adding more data to cs_main is sad!
7482020-07-02T20:49:56 *** promag has joined #bitcoin-core-dev
7492020-07-02T20:50:00 <sipa> i don't see why - if it's where it belongs
7502020-07-02T20:50:16 <sipa> if (nearly) all code accessing that data is already holding cs_main, it's the natural choice
7512020-07-02T20:50:34 <jnewbery> I'm afraid I've muddied the waters. sipa is right that orphan_work_set can stay per-peer while moving stuff out of net. I also happen to think that moving it out of per-peer is a good change.
7522020-07-02T20:51:04 <sipa> right, i'd be happy if we can have separate discussions about those two things
7532020-07-02T20:51:29 <aj> combining code cleanup refactors and behaviour changes seems like a bad idea in general to me
7542020-07-02T20:52:03 <jnewbery> aj: that's fair
7552020-07-02T20:52:34 <aj> (but not in specific for any time i do it, of course)
7562020-07-02T20:52:53 <jnewbery> if you don't like this, you're going to hate #19184. It changes behaviour and makes it a *lot* cleaner
7572020-07-02T20:52:56 <gribble> https://github.com/bitcoin/bitcoin/issues/19184 | Overhaul transaction request logic by sipa · Pull Request #19184 · bitcoin/bitcoin · GitHub
7582020-07-02T20:53:32 <aj> depends if the motivation is cleaning up code or changing the behaviour
7592020-07-02T20:54:34 <aj> (ie, if you're trying to change behaviour, and happen to clean up the code, great. if you're trying to clean up the code, and happen to change the behaviour, horrible)
7602020-07-02T20:54:37 <dongcarl> sipa: Is there any harm other than overhead if we keep the granular locks while moving things from CNode to PeerState?
7612020-07-02T20:54:53 *** sipsorcery has quit IRC
7622020-07-02T20:55:12 <aj> orphan_work_set currently doesn't have a lock of its own, i think?
7632020-07-02T20:55:14 <sipa> dongcarl: i think it may discourage improvements that require breaking the granular locks; otherwise no
7642020-07-02T20:55:40 <sipa> aj: all current accesses to it are protected by both cs_main and g_cs_orphans, i believe
7652020-07-02T20:56:15 <aj> sipa: ProcessMessages checks whether it's empty() before (and after) grabbing those locks
7662020-07-02T20:56:31 <sipa> good point
7672020-07-02T20:57:10 <sipa> not a problem, as net_processing's processmessages (and functions called by it) are the only ones accessing it
7682020-07-02T20:57:18 <sipa> but not a good state of affairs
7692020-07-02T20:57:21 <jnewbery> it's implictly safe because we can only call ProcessMessages() oncee at a time
7702020-07-02T20:58:45 <aj> i occassionally think it'd be nice to have dummy per-thread mutexes so we could say GUARDED_BY(ThreadMessageHandlerMutex) for stuff that's only ever called from a particular thread
7712020-07-02T20:58:56 <sipa> https://github.com/sipa/bitcoin/commit/8bfa52ecefe974349b48603a7e498d20bbe71239 <- moves it to net_processing, protects it with g_cs_orphans
7722020-07-02T21:00:01 *** corelax1 has quit IRC
7732020-07-02T21:00:34 <sipa> clang static analyzer is happy
7742020-07-02T21:01:28 <dongcarl> jnewbery: If we move application-level things from CNode to PeerState, are there cases where the code accessing the "things moved from CNode to PeerState" doesn't already hold cs_main?
7752020-07-02T21:01:47 <jnewbery> that's one of the motivations
7762020-07-02T21:01:55 <jnewbery> to reduce the scope of where we need to lock cs_main
7772020-07-02T21:02:28 <sipa> dongcarl: technically, yes
7782020-07-02T21:03:40 <sipa> jnewbery: yeah, i understand how you'd want to avoid the increase in cs_main like in the commit i linked above
7792020-07-02T21:04:04 <sipa> (in this case it could also just remain unprotected, but uh...)
7802020-07-02T21:05:17 <dongcarl> so wouldn't it be better to have granular locks if these cases of improvements exist? And perhaps in the future, we will have even more cases like these?
7812020-07-02T21:05:37 <jnewbery> dongcarl: here's one example: the UpdatedBlockTip callback here: https://github.com/bitcoin/bitcoin/blob/7027c67cac852b27c6d71489e4135fabdd624226/src/net_processing.cpp#L1329-L1336 doesn't take cs_main, but access CNode data
7822020-07-02T21:05:59 <sipa> i think having one giant net_processing lock is much better (and doesn't introduce cs_main locking)
7832020-07-02T21:06:19 <jnewbery> (nStartingHeight and the fields that PushBlockHash() accesses are in CNode)
7842020-07-02T21:06:50 <sipa> and also doesn't hurt performance in any way, because net processing is inherently single threaded
7852020-07-02T21:07:28 <jnewbery> that callback runs on the scheduler thread. Currently it can be run in parallel to anything else happening in net processing. If we add a cs_net_processing, I don't think it can do that any more
7862020-07-02T21:07:45 <sipa> jnewbery: interesting!
7872020-07-02T21:10:16 <sipa> jnewbery: though i think you can achieve the same by having a global sub-lock for the relevant data (which inside processmessages would be accessed with cs_net_processing + sublock, and in the callback with just the sublock)
7882020-07-02T21:10:24 <dongcarl> just for context: I'm interested in this cuz I've been looking at the async PNB stuff, which also introduces a PeerState class, and aims to reduce lock contention with that: https://github.com/bitcoin/bitcoin/pull/16323/commits/3a3b13e1988c319e35f42cd359b8fa36a3a2503d
7892020-07-02T21:11:56 <jnewbery> I don't think cs_net_processing is necessary if you have sublocks for all the different subsections of data. But yes, there are different ways you can slice this.
7902020-07-02T21:12:04 <sipa> agree
7912020-07-02T21:13:17 <sipa> and sublocks for different types of data may permit some limited concurrency too (if they're actually separate...), like having each thread check "which locks does the next message from peer X need? if those are busy, skip to next peer; otherwise, grab and process"
7922020-07-02T21:14:26 <sipa> but unfortunately that's hard to do in a clean way that accounts for locks needed by dependent modules (e.g. hard to make it skip things that need exclusive access to validation while validation is busy)
7932020-07-02T21:15:06 <jnewbery> yes, that does seem difficult
7942020-07-02T21:15:53 <sipa> of course, with cs_main reduced, it may be easier to turn validation into protected with readwrite locks... which would make it less of an issue too
7952020-07-02T21:20:01 *** nik-j has quit IRC
7962020-07-02T21:20:36 *** nik-j has joined #bitcoin-core-dev
7972020-07-02T21:22:01 *** RhodiumToad1 has joined #bitcoin-core-dev
7982020-07-02T21:22:13 *** nik-j has quit IRC
7992020-07-02T21:22:27 *** nik-j has joined #bitcoin-core-dev
8002020-07-02T21:24:01 *** bitcoin-git has joined #bitcoin-core-dev
8012020-07-02T21:24:02 <bitcoin-git> [bitcoin] meshcollider pushed 5 commits to master: https://github.com/bitcoin/bitcoin/compare/7027c67cac85...a24806c25d7a
8022020-07-02T21:24:02 <bitcoin-git> bitcoin/master 72f6bec Andrew Chow: rpc: show both UTXOs in decodepsbt
8032020-07-02T21:24:03 <bitcoin-git> bitcoin/master 5279d8b Andrew Chow: psbt: Allow both non_witness_utxo and witness_utxo
8042020-07-02T21:24:03 <bitcoin-git> bitcoin/master 4600479 Andrew Chow: psbt: always put a non_witness_utxo and don't remove it
8052020-07-02T21:24:05 *** bitcoin-git has left #bitcoin-core-dev
8062020-07-02T21:24:15 *** nik-j has joined #bitcoin-core-dev
8072020-07-02T21:24:26 *** bitcoin-git has joined #bitcoin-core-dev
8082020-07-02T21:24:26 <bitcoin-git> [bitcoin] meshcollider merged pull request #19215: psbt: Include and allow both non_witness_utxo and witness_utxo for segwit inputs (master...psbt-segwit-fixes) https://github.com/bitcoin/bitcoin/pull/19215
8092020-07-02T21:24:27 *** bitcoin-git has left #bitcoin-core-dev
8102020-07-02T21:27:30 *** marcoagner has quit IRC
8112020-07-02T21:41:54 *** promag has quit IRC
8122020-07-02T21:42:26 *** promag has joined #bitcoin-core-dev
8132020-07-02T21:45:51 *** AaronvanW has joined #bitcoin-core-dev
8142020-07-02T22:19:12 *** EagleTM has joined #bitcoin-core-dev
8152020-07-02T22:22:13 *** EagleTM has quit IRC
8162020-07-02T22:22:39 *** EagleTM has joined #bitcoin-core-dev
8172020-07-02T22:25:38 *** proofofkeags has quit IRC
8182020-07-02T22:25:53 *** mdunnio has quit IRC
8192020-07-02T22:26:13 *** proofofkeags has joined #bitcoin-core-dev
8202020-07-02T22:26:42 *** proofofkeags has quit IRC
8212020-07-02T22:26:47 *** justanotheruser has quit IRC
8222020-07-02T22:26:55 *** proofofkeags has joined #bitcoin-core-dev
8232020-07-02T22:40:30 *** mdunnio has joined #bitcoin-core-dev
8242020-07-02T22:40:32 *** mdunnio has quit IRC
8252020-07-02T22:41:07 *** mdunnio has joined #bitcoin-core-dev
8262020-07-02T22:41:12 *** EagleTM has quit IRC
8272020-07-02T22:41:36 *** EagleTM has joined #bitcoin-core-dev
8282020-07-02T22:44:08 *** justanotheruser has joined #bitcoin-core-dev
8292020-07-02T22:49:56 *** mdunnio has quit IRC
8302020-07-02T22:50:39 *** EagleTM has quit IRC
8312020-07-02T22:51:02 *** EagleTM has joined #bitcoin-core-dev
8322020-07-02T22:58:17 *** EagleTM has quit IRC
8332020-07-02T22:58:42 *** EagleTM has joined #bitcoin-core-dev
8342020-07-02T23:10:27 *** EagleTM has quit IRC
8352020-07-02T23:10:51 *** EagleTM has joined #bitcoin-core-dev
8362020-07-02T23:14:15 *** EagleTM has quit IRC
8372020-07-02T23:14:38 *** EagleTM has joined #bitcoin-core-dev
8382020-07-02T23:17:41 *** EagleTM has quit IRC
8392020-07-02T23:18:08 *** EagleTM has joined #bitcoin-core-dev
8402020-07-02T23:19:59 *** mdunnio has joined #bitcoin-core-dev
8412020-07-02T23:23:27 *** abd has joined #bitcoin-core-dev
8422020-07-02T23:24:14 *** mdunnio has quit IRC
8432020-07-02T23:24:47 *** abd has quit IRC
8442020-07-02T23:33:43 *** vasild has quit IRC
8452020-07-02T23:35:44 *** vasild has joined #bitcoin-core-dev
8462020-07-02T23:39:41 *** EagleTM has quit IRC
8472020-07-02T23:40:08 *** EagleTM has joined #bitcoin-core-dev
8482020-07-02T23:43:08 *** EagleTM has quit IRC
8492020-07-02T23:43:32 *** EagleTM has joined #bitcoin-core-dev
8502020-07-02T23:52:50 *** EagleTM has quit IRC