09:34:33 <sech1> #8752 needs some love again
09:34:53 <sech1> It's been dragging for too long
11:59:05 <selsta> sech1: I'll review it today
13:43:35 <selsta> sech1: did you closely review all the test related changes outside trezor tests?
13:50:16 <sech1> I reviewed all files. If you check comments from my initial review, I was asking some questions about those files. IIRC it was mostly renaming stuff in tests to make them work correctly again.
13:56:10 <selsta> I'm always a bit careful with merging changes to core_tests
14:02:04 <sech1> I'm not an expert in core tests though, here's where I asked it: https://github.com/monero-project/monero/pull/8752#pullrequestreview-1492566412
14:02:34 <sech1> "monero tests: fix tx create from sources, input locking. Originally, creating a synthetic transactions with chaingen could create a transaction with outputs that are still locked in the current block, thus failing chain validation by the daemon. Simple unlock check was added. Some buggy tests were fixed as well as new unlock-checking version of tx
14:02:34 <sech1> creation rejected those, fixes are simple - mostly using correct block after a rewind to construct a transaction"
14:04:38 <sech1> "Regarding the blk_5r question - I had to rewind so tests pass after fix of tx lock time."
14:16:57 <moneromoooo> That sucks. This really should be a separate commit, or it's unreviewable really.
14:18:57 <moneromoooo> (not to mention unbisectable/unrevertable)
14:28:51 <selsta> moneromoooo: so one commit for chaingen / core test related changes and the remaining trezor changes in the other commit?
14:30:14 <moneromoooo> I try to make one commit per fix when I can. It's *usually* not hard to commit a fix separately when you code a larger thing and you uncover some bug while doing so. Not *always* easy, but usually. And since this patch isn't really needed since it's just for trezor, it applies even more here.
14:31:04 <moneromoooo> Quite often I'll have a string of random fixes or small improvements, then the main patch that adds a feature. It makes it easier to review, bisect, revert piecemeal if needed.
14:31:40 <moneromoooo> For this particular patch, it might be harder to do if both modify the same code, I've not looked.
14:31:49 <moneromoooo> But then again, 41 files changed is good incentive not to look...
15:03:51 <selsta> he will try to split it up
16:34:07 <m-relay> <j​effro256:monero.social> Is it also possible to split the libUSB changes into its own PR ?
16:35:45 <selsta> the PR already has 160 comments, I feel bad asking for more changes at this point lol
16:36:07 <m-relay> <j​effro256:monero.social> True True
16:36:40 <m-relay> <j​effro256:monero.social> I want to see the core_tests modifications in a different PR, but I can live otherwise
16:37:06 <selsta> they will be in the same PR but a different commit since otherwise tests wouid fail
16:40:39 <m-relay> <j​effro256:monero.social> Fair enough
16:49:18 <moneromoooo> Well, a massive patch with unrelated things in it, should have known better. Though I know it's sometimes tempting.
17:16:05 <selsta> this is just the chaingen patch https://github.com/monero-project/monero/pull/8752/commits/056c9967036175e78b7ac1758d9a2943a9283bfc