💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1732691023)
Yes, we don't want a short circuit, though in practice `add_extra_compact_tx` should always be true at this point in the code. I've added a comment describing what the expected changes can be.

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2312348384)
Thanks @dergoegge @theStack @instagibbs! Addressed all comments.

> Slightly off-topic, but: on some commits like https://github.com/bitcoin/bitcoin/commit/4e58d84da95d97d64f563b5c3e116769cd9ff89b --color-moved=dimmed-zebra didn't detect any lines as move-only for me (with 2-3 exceptions that seem completely arbitrary), any suggestions for advanced git options and/or recommendations for other fancy diff tools that can cope with that?

I see that happen too :( very annoying. Sometimes if I se
...

👍 brunoerg approved a pull request: "bench: [refactor] iwyu"
(https://github.com/bitcoin/bitcoin/pull/30716#pullrequestreview-2263102976)
crACK fab0e834b8cf906c286ebbeed86eca8d65854f75

💬 hodlinator commented on pull request "refactor: Testnet4 - Replace uint256S("str") -> uint256{"str"}":
(https://github.com/bitcoin/bitcoin/pull/30721#issuecomment-2312424733)
> For the future: you could have made the commit here an actual scripted diff too.

Good point, thanks, still getting the hang of scripted diffs. Makes sense in case new stuff using `uint256S` gets merged into master before this.

💬 maflcko commented on pull request "refactor: Testnet4 - Replace uint256S("str") -> uint256{"str"}":
(https://github.com/bitcoin/bitcoin/pull/30721#issuecomment-2312441346)
> > For the future: you could have made the commit here an actual scripted diff too.
>
> Good point, thanks, still getting the hang of scripted diffs. Makes sense in case new stuff using `uint256S` gets merged into master before this.

A scripted-diff won't help here, because it is applied on top of the commit it is based on, which won't change when master is changed, unless you rebase on every push to master.

Moreover, I think scripted-diffs should only be used when it makes sense. As a
...

📝 maflcko opened a pull request: " lint: Speed up and fix flake8 checks "
(https://github.com/bitcoin/bitcoin/pull/30723)
The checks have many issues:

* Some checks that could in theory hide bugs are not applied -> Fix them and apply them going forward
* Some checks are redundant Python 2 checks, or of low value -> Remove them
* The checks are slow -> Speed them up from ~10 seconds to about ~20 milliseconds

📝 theStack opened a pull request: "test: add test for specifying custom pidfile via `-pid`"
(https://github.com/bitcoin/bitcoin/pull/30724)
This small PR adds test coverage for the `-pid` command line option, which allows to overrule the pid filename (`bitcoind.pid` by default). One can specify either a relative path (within the datadir) or an absolute one; the latter is tested using `self.options.tmpdir`. Note that the functional test file `feature_init.py` so far only contained a stress test; with this new sub-test added, both the description and the test name are adapted to be more generic.

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1732784821)
hm... `sa_len=0` is really strange because it doesn't know how to advance, there isn't even place for the size! (which it just read in the line above it!) it should at least break out of the loop in that case

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1732832981)
i think i get it, we're advancing in the wrong way; see https://opensource.apple.com/source/xnu/xnu-1504.9.17/bsd/net/rtsock.c.auto.html (some actual Apple source code)
```
#define ROUNDUP32(a) \
((a) > 0 ? (1 + (((a) - 1) | (sizeof(uint32_t) - 1))) : sizeof(uint32_t))
#define ADVANCE32(x, n) (x += ROUNDUP32((n)->sa_len))
...
dlen = ROUNDUP32(sa->sa_len);
m_copyback(m, len, dlen, (caddr_t)sa);
len += dlen;
...
```
ROUNDUP32:
- if the size is 0, advance by sizeof(uint32_t)
- o
...

💬 hodlinator commented on issue "ci: ConnectionRefusedError: [WinError 10061] No connection could be made because the target machine actively refused it":
(https://github.com/bitcoin/bitcoin/issues/30390#issuecomment-2312554026)
> Also #28509.

Thanks for the added context @hebasto! There seems to some agreement that upgrading to Python 3.12 would [fix](https://github.com/bitcoin/bitcoin/pull/28509#issuecomment-1726482147) the issue. However, I was unable to find a documented fix, do you have a link? Your [comment](https://github.com/bitcoin/bitcoin/issues/28411#issuecomment-1724233900) links to an issue with `subprocess.run` but it's a higher-level function implemented on top of `subprocess.Popen` which we use.

>
...

💬 maflcko commented on issue "ci: ConnectionRefusedError: [WinError 10061] No connection could be made because the target machine actively refused it":
(https://github.com/bitcoin/bitcoin/issues/30390#issuecomment-2312567392)
> There seems to some agreement that upgrading to Python 3.12 would [fix](https://github.com/bitcoin/bitcoin/pull/28509#issuecomment-1726482147) the issue. However, I was unable to find a documented fix, do you have a link?

This was just a wrong comment by myself. See https://github.com/bitcoin/bitcoin/pull/28509#issuecomment-1727885738

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2312583122)
i still intend to do this, but honestly i've kind of lost track of the scope. Can we first aim to fix the bugs here, then merge an implementation that at least doesn't have more problems than the current `libnatpmp` implementation?

Then later on we can make sure all edge cases with `bind`, `listenport`, routers assigning different ports, etc. are handled.

💬 furszy commented on pull request "test: add test for specifying custom pidfile via `-pid`":
(https://github.com/bitcoin/bitcoin/pull/30724#discussion_r1732866434)
nit:
why not declaring this custom param within the test function?

✅ laanwj closed a pull request: "depends: Remove Qt build-time dependencies"
(https://github.com/bitcoin/bitcoin/pull/29923)

💬 laanwj commented on pull request "depends: Remove Qt build-time dependencies":
(https://github.com/bitcoin/bitcoin/pull/29923#issuecomment-2312593547)
closing due to lack of interest

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2312603309)
ACK 6f6fc1464ad2f2981e7c3ff5d6492f60f89a6504

Attends to my concerns with the fuzz tests. Now in `txdownloadman_one_honest_peer` no "non-good" peer will ever deliver `TX_REAL`, ensuring that the test is actually testing what we want. Determinstic randomness was also given to the member txrequest modules.

Lastly, minimum time skips were increased to 100ms, meaning 100ms*100000=~166 minutes of simulated time. Since there are up to 8+120=128 peers, this means each peer can stall out the node
...

💬 hodlinator commented on pull request "refactor: Testnet4 - Replace uint256S("str") -> uint256{"str"}":
(https://github.com/bitcoin/bitcoin/pull/30721#issuecomment-2312604054)
> A scripted-diff won't help here, because it is applied on top of the commit it is based on, which won't change when master is changed, unless you rebase on every push to master.

Couldn't CI at least run scripted diffs when the PR they belong to gets merged into master - so it could be caught post-merge?

💬 cryptoquick commented on issue "Fatal LevelDB error: Corruption: block checksum mismatch on Linux ext4 SATA SSDs":
(https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2312605120)
I'm trying to check my RAM using memtest86, but I'm having trouble booting into that too. Give me a bit to troubleshoot.

💬 hebasto commented on pull request "fuzz: Add missing fuzz targets to cmake build":
(https://github.com/bitcoin/bitcoin/pull/30712#issuecomment-2312612637)
> Can be tested via:
>
> ```
> PRINT_ALL_FUZZ_TARGETS_AND_ABORT=1 ./bld-autot/src/test/fuzz/fuzz > /tmp/f_autot
> PRINT_ALL_FUZZ_TARGETS_AND_ABORT=1 ./bld-cmake/src/test/fuzz/fuzz > /tmp/f_cmake
> diff --unified /tmp/{f_autot,f_cmake}
> ```

Such a check can be automated: https://github.com/hebasto/bitcoin/commits/pr30712_amended.

💬 maflcko commented on pull request "refactor: Testnet4 - Replace uint256S("str") -> uint256{"str"}":
(https://github.com/bitcoin/bitcoin/pull/30721#issuecomment-2312650978)
When a commit from a pull request (which may contain a scripted-diff) is merged into another branch, the commit itself does not change, so it won't affect the execution or the result of the scripted-diff in that commit.

In theory, the commits could be rebased/cherry-picked instead of merged. However, that'd make auditing harder, because it makes it easier to inject malicious code accidentally or intentionally. Also, it wouldn't help here, because then the scripted diff check may permanently f
...