💬 S3RK commented on pull request "wallet: return error msg for "too-long-mempool-chain"":
(https://github.com/bitcoin/bitcoin/pull/24845#discussion_r1141752785)
1) I think this should respect `fRejectLongChains` flag. If user explicitly asked us to ignore long-chain constraints, then we should respect that.

2) If `fRejectLongChains` flag is set (the default) then the most lenient filter has `max_ancestors/2` and `max_descendants/2`. So this condition could never be hit.

In summary I don't yet understand how one can hit a message described in #23144

💬 S3RK commented on pull request "wallet, tests: Expand and test when the blank wallet flag should be un/set":
(https://github.com/bitcoin/bitcoin/pull/25634#discussion_r1141776601)
I have a different perspective. The flag was provided during wallet creation and captures the intent of the user.

> The blank flag is used only to indicate to that code that the wallet is intentionally blank and should not be setup as a new wallet.

I'd like to take this one step further and argue that "The blank flag is used only to indicate to that code that the wallet is intentionally blank and **the user is responsible for managing the keys in this wallet manually**." If user created wa
...

💬 S3RK commented on pull request "wallet, tests: Expand and test when the blank wallet flag should be un/set":
(https://github.com/bitcoin/bitcoin/pull/25634#issuecomment-1475818349)
Concept ACK. But approach wise I'd like us to preserve the intent of the user that the wallet is intended as `manual` (we should a better term). Would it be hard to create a new flag to persist that intent and respect that (e.g on encryption)?

💬 MarcoFalke commented on pull request "Move log messages: tx enqueue to mempool, allocation to blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27277#issuecomment-1475912615)
CI network error https://cirrus-ci.com/task/4864754720702464?logs=ci#L61:

`TimeoutError: [Errno 110] Connection timed out`

💬 MarcoFalke commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1475931039)
re-ACK ff6c0abeea00deffcaeaf3b02092110d6895b1c0 🦀

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK ff6c0abeea00deffcaea
...

💬 MarcoFalke commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1141877181)
> Commits have been reorganized.

No they haven't?

💬 MarcoFalke commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1141878435)
The commit that fixes the use-after-free should come with a commit message to say that it fixed the bug

💬 MarcoFalke commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1475934980)
nit in https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1141878435 is still unfixed?

💬 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