💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1141893047)
> cs_main

Hmm, that sounds like you are making it harder to remove cs_main.

I am not sure it the alternative are better, but my brain enjoys the thought of having only to think about a single blockman object at a time. Ideas:

* Use `findBlock` from the node interface. The overhead should only be another map lookup by the block hash? You can ask Ryan to confirm.
* Pass down a blockman ref into the zmq stuff (massive change)
* Add a new global alias for zmq to point to the single existi
...

💬 fanquake commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1141897562)
> I may have missed some nuance here. To me it seemed acceptable, because functions are protected by cs_main. For const functions likeReadBlockFromDisk this seems safe to me

Note that according to the PR description in #27006 there is currently at least one `cs_main` related bug in `ReadBlockFromDisk`.

💬 josibake commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1141899708)
styling nit: remove the extra line at the end of the file

💬 josibake commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1141838493)
per the functional test README:

```
- If more than one name from a module is needed, use lexicographically sorted multi-line imports
in order to reduce the possibility of potential merge conflicts.
```

so this should be

```suggestion
from test_framework.segwit_addr import (
bech32_decode,
convertbits,
encode_segwit_address,
)
```

💬 josibake commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1141903161)
styling nit: two blank lines between functions, per PEP8 guidlines

💬 josibake commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1141899143)
since you are already using the `encode_segwit_address` function, it might be better to also use the `decode_segwit_address` function here instead of duplicating the logic.

something like:

```suggestion
hrp = address[:2]
version, payload = decode_segwit_address(hrp, address)
return version, bytearray(payload)
```

💬 josibake commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1141903949)
styling nit: two blank lines between functions, per PEP8 guidelines

💬 josibake commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1141909051)
styling nit: should be lexicographically sorted and add a comma at the end of each line (even if it is the last one)

💬 josibake commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1141905124)
also, if you end up taking my suggestion to use `decode_segwit_address`, you can remove the `bech32_decode, convertbits` imports

💬 flack commented on pull request "refactor: wallet, do not translate init options names":
(https://github.com/bitcoin/bitcoin/pull/25666#discussion_r1141928781)
Strictly speaking, the middle sentence is a bit redundant (and also contains the option name)

💬 TheCharlatan commented on pull request "refactor: Extract util/fs from util/system":
(https://github.com/bitcoin/bitcoin/pull/27254#issuecomment-1476003813)
Rebased b22cf8d563a469e29c9942b4fd9f93351d8b9766 -> cc662d09386e50eb92a7ccbaa1edff62fd8cdd44 ([splitSystemFs_4](https://github.com/TheCharlatan/bitcoin/commits/splitSystemFs_4) -> [splitSystemFs_5](https://github.com/TheCharlatan/bitcoin/commits/splitSystemFs_5), [compare](https://github.com/TheCharlatan/bitcoin/compare/splitSystemFs_4..splitSystemFs_5)) to fix conflicts and update the commit message to exclude the scripted diff.

📝 MarcoFalke opened a pull request: "test: Fix TypeError (expected str instance, bytes found) in wait_for_debug_log"
(https://github.com/bitcoin/bitcoin/pull/27280)
null

💬 MarcoFalke commented on pull request "test: Fix TypeError (expected str instance, bytes found) in wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/27280#issuecomment-1476011366)
Can be tested with:

```diff
diff --git a/test/functional/feature_init.py b/test/functional/feature_init.py
index 70a802dc58..1d1c6a64b6 100755
--- a/test/functional/feature_init.py
+++ b/test/functional/feature_init.py
@@ -52,7 +52,7 @@ class InitStressTest(BitcoinTestFramework):
assert_equal(200, node.getblockcount())

lines_to_terminate_after = [
- b'Validating signatures for all blocks',
+ b'.Validating signatures for all blocks',

...

🤔 dergoegge requested changes to a pull request: "Log new headers"
(https://github.com/bitcoin/bitcoin/pull/27278)
Concept ACK

Prefer this over #27276 as well I think.

Maybe add the peer id to the log locations as well?

💬 dergoegge commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1141510303)
I think this `LogPrintf` call isn't safe because the work on the header has not actually been checked, that first happens in `ProcessNewBlockHeaders` below (by "safe" i mean that this location could be used to fill our disk with log messages).

At this point the header is only claiming to have enough work (i.e. `prev_block->nChainWork + CalculateHeadersWork({cmpctblock.header}) >= GetAntiDoSWorkThreshold()`) but the actual PoW check happens later on.

You can move this log location below the
...

💬 fanquake commented on pull request "Log successful AcceptBlockHeader()":
(https://github.com/bitcoin/bitcoin/pull/27276#issuecomment-1476018573)
Closing in favour of #27278.

✅ fanquake closed a pull request: "Log successful AcceptBlockHeader()"
(https://github.com/bitcoin/bitcoin/pull/27276)

👍 alexsio27444 approved a pull request: "Log new headers"
(https://github.com/bitcoin/bitcoin/pull/27278)

💬 josibake commented on pull request "RPC: Fix fund transaction crash when at 0-value, 0-fee":
(https://github.com/bitcoin/bitcoin/pull/27271#discussion_r1141953882)
grammar nit:

```suggestion
// This can only happen if the feerate is 0, the requested destinations are value of 0 (e.g OP_RETURN),
// and there are no pre-selected inputs. This will result in a 0-input transaction, which is consensus-invalid
```

👍 alexsio27444 approved a pull request: "refactor: Add BlockManager getters"
(https://github.com/bitcoin/bitcoin/pull/26900)