💬 ryanofsky commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1954970365)
> I agree when it comes to `assert()`s in C/C++, guess I haven't fully transferred that to Python yet, but this discussion helped. What do you think of doing a scripted diff renaming our `assert_*()`-functions and their usage to `check_*()` to signal that they won't be optimized out?

IMO a renaming would not be a good idea just because of all the conflicts it would create. Also I think it overlapping names for assert functions and the assert statement are less of a problem in python than they
...

💬 fanquake commented on pull request "depends: add missing Darwin objcopy":
(https://github.com/bitcoin/bitcoin/pull/31840#issuecomment-2657346395)
> even though it's annoying because it's not actually necessary, I don't see much harm in setting it. @fanquake ?

I'm just not sure of us having to setup/provide tools because CMake thinks they should be available, when we don't use them. iirc one of the issues with `install-name-tool` was that it was only available version-suffixed on certain distros (even when the main llvm install was not suffixed), making it hard to find.

> This required a similar hack for https://github.com/bitcoin/b
...

💬 theuni commented on pull request "cmake: add a component for each binary":
(https://github.com/bitcoin/bitcoin/pull/31844#discussion_r1955002067)
Heh, testing shows `GNUInstallDirs` is actually coming in from secp and we don't need it at all in our project. That's pretty gross of CMake...

But still, I'll remove this one and leave the other one there for self-documentation.

💬 ryanofsky commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1955022152)
re: https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1954786986

In commit "Add waitNext() to BlockTemplate interface" (86449088b703d2110edaacbd6d1f1386916a3773)

> I incorporated part of this comment. Note that some callers may just not be interested in fee changes, so I mentioned that as well.

Thanks, I do have two more suggestions:

- In "A caller may not be interested in templates with higher fees, but determining whether fee_threshold is reached is also expensive," can you
...

👍 ryanofsky approved a pull request: "Add waitNext() to BlockTemplate interface"
(https://github.com/bitcoin/bitcoin/pull/31283#pullrequestreview-2615870327)
Code review ACK 16b18f845313f25bbcdea122baa753b570add7d0. Just various suggested changes since last review.

> (there seems to be a delay in Github processing my latest push)

Can confirm I see "Processing Updates" "Recent push is still being processed, and will show up in a bit" at the top of this PR.

💬 theuni commented on pull request "cmake: add a component for each binary":
(https://github.com/bitcoin/bitcoin/pull/31844#issuecomment-2657416328)
Addressed @stickies-v's review.

Only change of interest is changing the name of the `Kernel` component to `bitcoinkernel` rather than `libbitcoinkernel` as it was before.

💬 theuni commented on pull request "depends: add missing Darwin objcopy":
(https://github.com/bitcoin/bitcoin/pull/31840#issuecomment-2657423789)
> > even though it's annoying because it's not actually necessary, I don't see much harm in setting it. @fanquake ?
>
> I'm just not sure of us having to setup/provide tools because CMake thinks they should be available, when we don't use them. iirc one of the issues with `install-name-tool` was that it was only available version-suffixed on certain distros (even when the main llvm install was not suffixed), making it hard to find.

Ah right, I'd forgotten that bit. Blah.

Yeah, ok, never
...

💬 ryanofsky commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-2657453951)
Rebased 47a872236e070814ad74922d3f8a653e1c6af968 -> d2ceb2e0735a2c8343f8316b55fac55323aba62c ([`pr/libexec.2`](https://github.com/ryanofsky/bitcoin/commits/pr/libexec.2) -> [`pr/libexec.3`](https://github.com/ryanofsky/bitcoin/commits/pr/libexec.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/libexec.2-rebase..pr/libexec.3)) due to conflict with #31834

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2657455048)
It seems Github never around to my last push. The new one was picked up immediately. It incorporates the two suggestions.

💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2657465365)
Alright, we're back to two commits. Let's see how CI fares.

💬 ryanofsky commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-2657467856)
Rebased 99f2f335b6a8c2b5fe518a0c0056eb2a42c0496b -> 9194e461518e6b843c1a707ba1b10ab7a000e096 ([`pr/mine.15`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.15) -> [`pr/mine.16`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.16), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.15-rebase..pr/mine.16)) due to conflict with #31834. Also added status line for the executable as suggested

💬 ryanofsky commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2657469079)
Rebased 99f2f335b6a8c2b5fe518a0c0056eb2a42c0496b -> 9194e461518e6b843c1a707ba1b10ab7a000e096 ([`pr/mine.15`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.15) -> [`pr/mine.16`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.16), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.15-rebase..pr/mine.16)) due to conflict with #31834. Also added status line for the executable as suggested

💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1955067561)
Good point, `getTip()` doesn't wait.

👍 ryanofsky approved a pull request: "Add waitNext() to BlockTemplate interface"
(https://github.com/bitcoin/bitcoin/pull/31283#pullrequestreview-2615975522)
Code review ACK 2d65225c4696dcc8312d546eb67f4eae11b9b8fe, just updating fee_threshold comment since last review

🤔 theuni reviewed a pull request: "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop"
(https://github.com/bitcoin/bitcoin/pull/31826#pullrequestreview-2615930348)
Code review ACK 585aba6eec858e5b1411ae9a8684ef8f82a7e435. Untested. A few (non-blocking) notes, assuming this works as-is.

1. It's unclear to me under what scenario the call might fail. OpenSSL does the same loop, I assume that's why it's done here? I guess it's pretty unlikely that it passes the <10 failures check at startup, then fails lots of times during execution.
2. The code duplication is unfortunate. I would've preferred to see a single factored-out function that takes a reference to
...

💬 theuni commented on pull request "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1955041233)
In case it helps other reviewers... no need for static for these because of the anon namespace.

💬 theuni commented on pull request "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1955045279)
I think `ok` and `test` should be initialized to sane values because some sanitizers (and reviewers :p) aren't clever enough to know if they're guaranteed to be set by the asm.

edit: I guess the same goes for the other copy of this.

💬 ryanofsky commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1955078544)
re: https://github.com/bitcoin/bitcoin/pull/31283/commits/dcb75cbae61918a13310e3e22190c088d9b7605c#r1937644528

> I'd really like this to be easy to use for a Rust client as early as possible. It seems better to revert to the magic value for now.

To make this easy for rust (or other) cilents using this interface you can assign these fields default values. Maybe define MAX_MONEY and MAX_TIMEOUT constants using syntax shown https://capnproto.org/language.html#constants and assign them as defa
...

💬 laanwj commented on pull request "test: Remove stale gettime test":
(https://github.com/bitcoin/bitcoin/pull/31846#issuecomment-2657497175)
Sure. It was good to have this test at the time to prove the issue was fixed. But it's been a decade, it's super unlikely that this specific toolchain ABI compatibility issue will ever pop up again in our builds, we can't keep the tests for every possible eventuality.

Generally we should have tests that test our code, we can't assure the sanity of the entire OS and system libraries.

💬 TheCharlatan commented on pull request "Cleanups to port mapping module post UPnP drop":
(https://github.com/bitcoin/bitcoin/pull/31157#discussion_r1955087765)
This comment seems to be lacking context now.