💬 maflcko commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#discussion_r1592685484)
> Therefore the only way I could concieve of not linting random directories in the root src dir

If this is too difficult, it does not need to be implemented for now. The risk should be minimal, as `mlc` should only have read-only access? And if it were to fail for someone that put random other folders into the root, they can always skip the check?

💬 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.