💬 S3RK commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1593513968)
nit: do you want to add assert for the comment above? It's not obvious that the statement holds given variable declarations are hundreds of lines above.

the comment:
"Negative waste when the long term fee is greater than the current fee and change_cost < - (inputs * (fee - long_term_fee))"

💬 S3RK commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1593517046)
nit: could use `exact_target - excess` if you introduce `exact_target` var

💬 S3RK commented on pull request "tests: improve wallet multisig descriptor test and docs":
(https://github.com/bitcoin/bitcoin/pull/29154#discussion_r1593544047)
In this case `random.sample` shuffles the order of participants, but I don't see how it matters since all participants are equivalent anyway

💬 S3RK commented on pull request "tests: improve wallet multisig descriptor test and docs":
(https://github.com/bitcoin/bitcoin/pull/29154#issuecomment-2099956492)
The change looks good to me modulo addressing comments from achow101.

💬 Sjors commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2099974737)
> [...] use a recent block hash as a provably unspendable public key (instead of all zeros).
>
> Well, we now have a genesis block with the hash in the message and the all zeros public key. I don't think that makes a difference anymore?

I find it more elegant, but only if we need another genesis block.

> In practice today that probably means pre-mining a handful of blocks to add the test cases and then checkpointing the end of the pre-mine.

That makes sense if we want the test cases
...

💬 laanwj commented on pull request "net: Replace ifname check with IFF_LOOPBACK in Discover":
(https://github.com/bitcoin/bitcoin/pull/29984#issuecomment-2100006946)
Thanks for checking! it's kind of a belt-and-suspenders check: we only look for routable addresses in Discover, so unless someone assigned a routable address to the loopback interface-which would be really strange-it doesn't actually influence the result.

🚀 fanquake merged a pull request: "test: added test coverage to loadtxoutset could not open file"
(https://github.com/bitcoin/bitcoin/pull/30053)

💬 emsit commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2100018346)
Is there a plan to replace the path '/testnet3' with '/testnet'? Due to backward compatibility, that might not be a good idea.

https://github.com/fjahr/bitcoin/commit/37e202adc1c734740104c6a5e1d2bd021102764e#diff-583f605f372805d0bc87ebfc893f835b3f66746f560611e682bd917de17586d2
https://github.com/fjahr/bitcoin/commit/37e202adc1c734740104c6a5e1d2bd021102764e#diff-723b9eeeb819047ef03f827ee8a3ccfef48ce7ef6a340a068a68b551993fe70b

💬 paplorinc commented on pull request "test: use assert_greater_than util":
(https://github.com/bitcoin/bitcoin/pull/30019#discussion_r1593622473)
`idx` can equal `MAX_NODES` according to the code

💬 TheCharlatan commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#issuecomment-2100025531)
Concept ACK on removing the warnings globals.

💬 josibake commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1593657407)
Oops! Should have ran it myself before including it. I have a slight preference for keeping the sage code, since its nice to be able to run that snippet and get the same value as a way of verifying. That being said, "be able to run that snippet" implies keeping the comment up to date, but I doubt that will be much (if any) of a maintenance burden.

Also think its more correct to output the x,y coordinate pair since in the comment we are referring to `NUMS_H` as a point, whereas the x-only valu
...

💬 josibake commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1593668308)
Updated to fix the hex decoding. Went ahead and left the (x,y) print out, but can drop the y-value or drop the whole sage snippet if others feel strongly.

✅ maflcko closed a pull request: "build: Require libc++-16 or later"
(https://github.com/bitcoin/bitcoin/pull/29077)

📝 maflcko opened a pull request: "ci: Roll clang in test-each-commit task"
(https://github.com/bitcoin/bitcoin/pull/30060)
Needed for https://github.com/bitcoin/bitcoin/pull/29077#issuecomment-2099704210

💬 theStack commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1593731959)
Agree that keeping the sage code and printing out the full point makes sense w.r.t. your point vs byte encoding argument. (By the way, at least in our test-framework implementation of GE, [`lift_x` always produces a point with even y coordinate](https://github.com/bitcoin/bitcoin/blob/43a66c55ec8770cf7c21112aac9b997f3f2fb704/test/functional/test_framework/crypto/secp256k1.py#L256), but no idea if this is also the case in the GE of sage.)

👍 theStack approved a pull request: "crypto: add `NUMS_H` const"
(https://github.com/bitcoin/bitcoin/pull/30048#pullrequestreview-2045153103)
Code-review ACK a5867a393cacbe4b2e0de51b842362754e98ab42

💬 maflcko commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#discussion_r1593737753)
nit: It is already a `Vec`, so no need to iter+collect, no?

💬 maflcko commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#discussion_r1593739133)
nit: Can also use `.args([...])` for less code?

💬 maflcko commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#discussion_r1593737106)
nit: Could remove this, as the URL is below as well.

💬 maflcko commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#discussion_r1593736433)
check_output is a helper that assumes the command exists, so I don't think it will work here, no?

Ref:

```
thread 'main' panicked at src/main.rs:23:28:
command error: Os { code: 2, kind: NotFound, message: "No such file or directory" }
stack backtrace:
0: rust_begin_unwind
...
```

I think in a previous version you had a check for `NotFound` directly, so you could restore that?