💬 GregTonoski commented on issue "bitcoind shouldn't fail to progress with synchronization: endless [leveldb] Generated table ... logs":
(https://github.com/bitcoin/bitcoin/issues/31882#issuecomment-2666920246)
> If it's just (very) slow, that may be due to low I/O speed of your disk

Disagree. The alleged cause is not supported by the data:
%iowait = 30.51%
kB_read = 21 624 950
kB_wrtn = 10 959 841
Only 31 blocks (~120 MB) downloaded during the time (c.a. 90 minutes).

💬 GregTonoski commented on issue "bitcoind shouldn't fail to progress with synchronization: endless [leveldb] Generated table ... logs":
(https://github.com/bitcoin/bitcoin/issues/31882#issuecomment-2666927835)
> Does it happen on a different filesystem than exfat? Does it happen on a different filesystem hardware, maybe something that is faster? (Can you benchmark the hardware?)

Yes, the same occurs in the same environment except NTFS instead of exFAT and HDD instead of SD.

👍 ryanofsky approved a pull request: "refactor: Remove redundant and confusing calls to IsArgSet"
(https://github.com/bitcoin/bitcoin/pull/31896#pullrequestreview-2625043669)
Code review ACK fa3fb3c23fae287b161112b9b04cf0937a1051c6. Nice refactoring to get rid of pointless complexity and repetition in this code.

💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1960600995)
Updated the commit message.

It's not a problem before, and not a problem now, because all of the things `SignPSBTInput` would fail on are checked by its callers.

💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1960601469)
Changed `RemoveUnnecessaryTransactions` to take sighash type as optional in this commit as well.

💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1960601610)
Done

💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1960602180)
This is all explained by the comment 2 lines up.

Took the suggestion, changed the comment to be less repetitive of things already said.

💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1960602709)
This is already explained by the comment a few lines above.

Added a less repetitive comment.

💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1960605964)
Fixed and reworded the comment.

`SIGHASH_ANYONECANPAY` does not commit to any input information, so none of the protections that allow us to drop the `non_witness_utxo` apply.

💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1960606092)
Fixed

👍 ryanofsky approved a pull request: "refactor: CLI cleanups"
(https://github.com/bitcoin/bitcoin/pull/31887#pullrequestreview-2625099756)
Code review ACK 3252f3bab02605129a568112f650ef3cb29703b7. All changes look good. No real opinion on the third commit, seems fine to keep or drop

💬 ryanofsky commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1960634114)
re: https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1959923000

Mostly agree with this suggestion. It would be nice to remove this `IsArgSet` call (in a separate commit or PR), because like [many](https://github.com/bitcoin/bitcoin/pull/30529) [other](https://github.com/bitcoin/bitcoin/pull/31212) `IsArgSet` calls, this one introduces a bug and doesn't handle negation properly. A common sense interpretation of `-nogetinfo` would be to run the getinfo command, not to run it.

Only p
...

💬 ismaelsadeeq commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1960644463)
@Sjors , does `tip_changed` ever become `true` at line 956 and we return from there? I think it will always be false hence can be deleted.

1. When we first enter this scope, the predicate is executed before blocking. If `tip_changed` is updated to `true`, it is returned. And when the predicate returned value is `true`, we do not wait; instead, we proceed to build and return a block template.

2. In subsequent executions of the predicate `tip_changed` will be `false` and we will check if the
...

💬 ismaelsadeeq commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1960455380)
style nit:
Break line to be more readable according to https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c

💬 ismaelsadeeq commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1960648041)
We can generate a template before a tick

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

Agree it looks like this line can be deleted. Also scope of `tip_changed` can be reduced so it is inside the do..while loop.

💬 yancyribbens commented on pull request "refactor: remove redundant `for` constructs":
(https://github.com/bitcoin/bitcoin/pull/31891#issuecomment-2667030347)
@l0rinc agreed, I'll drop it. However, I appreciate the more in depth explanation since this helps me know for the future so I don't make the mistakes. And yes, I agree that the comment `This refactor is a no-brainer` in retrospect was poor wording and I could have done better overall in a few ways. Anyway, thanks again for the detailed feedback.

💬 furszy commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1960663432)
It was a leftover from a previous implementation. But see https://github.com/bitcoin/bitcoin/pull/27469.

💬 furszy commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1960664340)
Ended up changing the testing approach, preferring functional over unit tests. But thanks.

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2667038911)
Updated 6f0405177f89377ed272bae42becec1c75b8943c -> 185c6a416df679ddc7b38750aa4d181c0b043d5b ([`pr/wrap.21`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.21) -> [`pr/wrap.22`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.22), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/wrap.21..pr/wrap.22)) just incorporating some python cleanups from #31866