💬 vasild commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1592701864)
I set `dstaddr` and `dstport` here in `connection_made()` because when accepting connections (from the python node point of view) they are set to `"0"` and `0` respectively (in `master`):

https://github.com/bitcoin/bitcoin/blob/ef09f535b7b540d997f8c2bfa67b3386711bc8b4/test/functional/test_framework/p2p.py#L203

https://github.com/bitcoin/bitcoin/blob/ef09f535b7b540d997f8c2bfa67b3386711bc8b4/test/functional/test_framework/p2p.py#L182-L186

When I add those asserts I get:
```
assert s
...

💬 vasild commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1592706360)
No, I just picked some value that looks reasonable, anything would work. For example `1` or `5` would still work but does not look reasonable because it will cause a lot of iteration loops.

💬 vasild commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1592709053)
I slightly prefer to keep the array unmodified in case the data in it is needed at some later point (for logging or something).

💬 willcl-ark commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#discussion_r1592709785)
Yes, I think this is probably the best approach too. I'll make those changes and push here soon (tm)

💬 luke-jr commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#discussion_r1592709382)
Is there a reason to name it something different here? Especially while the option is still broken and doesn't actually limit datacarriers?

💬 maflcko commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-2098816463)
> [clusterfuzz-testcase-mocked_descriptor_parse-5224675118481408.bin.not.txt](https://github.com/bitcoin/bitcoin/files/13877884/clusterfuzz-testcase-mocked_descriptor_parse-5224675118481408.bin.not.txt)

Same for the non-mocked one:

[clusterfuzz-testcase-minimized-descriptor_parse-5888623437217792.bin.txt](https://github.com/bitcoin/bitcoin/files/15238132/clusterfuzz-testcase-minimized-descriptor_parse-5888623437217792.bin.txt)


![Screenshot from 2024-05-07 18-05-00](https://github.com/
...

⚠️ hebasto opened an issue: "build: Unaligned libsecp256k1 flags in debug builds"
(https://github.com/bitcoin/bitcoin/issues/30055)
When configured with `--enable-debug`, C++ code compiles with `-O0 -g3`. However, libsecp256k1 code compiles with `-g -O2`.

⚠️ jdom1824 opened an issue: "(EDITED) How was the average size of blk*.data chosen?"
(https://github.com/bitcoin/bitcoin/issues/30056)
(EDITED)

List of implementation changes:
- new database layout:
- 2 leveldb's (coins/ and blktree/ subdirs), replacing blkindex.dat
- separate directory (blocks/) with block data (in the usual format, but smaller files) and undo data
- database keys are of the form (char,key) instead of (string,key) for reasons of compactness
- there is no txid-to-diskpos index anymore, only blkid-to-diskpos and txid-to-unspent-outputs
- this makes getrawtransaction only work on unspent outputs (
...

💬 davidgumberg commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1592765102)
nit:
```suggestion
LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "pcp: Requesting port mapping for addr %s port %d from gateway %s\n", bind.ToStringAddr(), port, gateway.ToStringAddr());
```

💬 brunoerg commented on issue "`keypoolrefill` doesn't fill keypool to specified parameter":
(https://github.com/bitcoin/bitcoin/issues/29924#issuecomment-2098939485)
I think you have more than one `ScriptPubKeyManager`. When you call `keypoolrefill ` it will top up every spkm. Did you check `listdescriptors`?

💬 luke-jr commented on pull request "guix: build with glibc 2.31":
(https://github.com/bitcoin/bitcoin/pull/29987#issuecomment-2098941527)
Is there a benefit to this? Just dropping patches?

💬 luke-jr commented on pull request "guix: fix suggested fake date for openssl-1.1.1l":
(https://github.com/bitcoin/bitcoin/pull/29999#issuecomment-2098953741)
Is it possible to use a UTS namespace to spoof the clock just for the build?

💬 luke-jr commented on pull request "dbwrapper: Bump LevelDB max file size to 128 MiB to avoid system slowdown from high disk cache flush rate":
(https://github.com/bitcoin/bitcoin/pull/30039#issuecomment-2098989028)
If there's no drawbacks, why not go even larger?

💬 hebasto commented on issue "ci: Enable bpfcc-tools":
(https://github.com/bitcoin/bitcoin/issues/29804#issuecomment-2098990473)
> > Wait for 24.04 GHA image, Move the task over to that image
>
> Still waiting for it to appear here: https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners/about-github-hosted-runners#standard-github-hosted-runners-for-public-repositories

The wait is over: https://github.com/actions/runner-images/releases/tag/ubuntu24%2F20240430.1

💬 theStack commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1592872178)
This code snippet doesn't work for me on Sage 9.5 (see https://github.com/BlockstreamResearch/secp256k1-zkp/pull/292). Also, we only need to output the x-coordinate here:
```suggestion
G2 = EllipticCurve ([F (0), F (7)]).lift_x(F(int(hashlib.sha256(bytes.fromhex(G_DER)).hexdigest(),16)))
print('%x' % G2.xy()[0])
```
(That said, I think it's also completely fine to not include any sage code here and instead just keep the first paragraph.)

💬 luke-jr commented on pull request "Bugfix - don't allow multiple dialogs for same tx in TransactionView":
(https://github.com/bitcoin-core/gui/pull/817#issuecomment-2099010611)
Why?

👍 ryanofsky approved a pull request: "blockstorage: Separate reindexing from saving new blocks"
(https://github.com/bitcoin/bitcoin/pull/29975#pullrequestreview-2041313158)
Code review ACK 194e84accced947ef63c6db389bc62a2b58cffa3. I left a lot of comments, but everything looks right here and the code is a lot nicer than before.

💬 ryanofsky commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1591338361)
In commit "validation, blockstorage: Separate code paths for reindex and saving new blocks" (a17eacab1f8790afc5f89ba2ee3e34da4c9369e1)

It looks like previously there would have been an error here if `dbp->IsNull()` was true, and now there will not be an error. This is probably a good change, since AcceptBlock should not be looking at block positions, just passing them on.

💬 ryanofsky commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1591463595)
In commit "blockstorage: Add Assume for fKnown / snapshot chainstate" (d6b0bb6dd0d26f3e10386d1afbafd8d52a12b2c5)

This comment seems really disconnected from the statement below it, because the statement does not mention reindexing or the snapshot chainstate at all. I left a suggestion to improve the comment below.

💬 ryanofsky commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1591477885)
In commit "blockstorage: split up FindBlockPos function" (a2ae0a33c7c30678721d7e7d37d8e6170b013383)

Would be helpful to clarify with meaning of `true` with `/*fFinalize*/=true`