💬 brunoerg commented on pull request "rpc, test: `addnode` improv + add test coverage for invalid command":
(https://github.com/bitcoin/bitcoin/pull/26366#discussion_r1281912594)
Done

💬 brunoerg commented on pull request "rpc, test: `addnode` improv + add test coverage for invalid command":
(https://github.com/bitcoin/bitcoin/pull/26366#issuecomment-1662221927)
Force-pushed addressing @jonatack's review. Addressed:

- https://github.com/bitcoin/bitcoin/pull/26366#discussion_r1281091853
- https://github.com/bitcoin/bitcoin/pull/26366#discussion_r1281087542
- https://github.com/bitcoin/bitcoin/pull/26366#discussion_r1281083516

🤔 jonatack reviewed a pull request: "rpc, test: `addnode` improv + add test coverage for invalid command"
(https://github.com/bitcoin/bitcoin/pull/26366#pullrequestreview-1559045632)
ACK f52cb02f700b58bca921a7aa24bfeee04760262b

Two non-blocking suggestions.

💬 jonatack commented on pull request "rpc, test: `addnode` improv + add test coverage for invalid command":
(https://github.com/bitcoin/bitcoin/pull/26366#discussion_r1281931598)
<details><summary>suggestion while touching this</summary><p>

```diff
- if (command == "onetry")
- {
+ if (command == "onetry") {
CAddress addr;
connman.OpenNetworkConnection(addr, /*fCountFailure=*/false, /*grantOutbound=*/nullptr, node_arg.c_str(), ConnectionType::MANUAL);
return UniValue::VNULL;
- }
-
- if (command == "add")
- {
+ } elsif (command == "add") {
if (!connman.AddNode(node_arg)) {
throw JSONRPCEr
...

💬 jonatack commented on pull request "rpc, test: `addnode` improv + add test coverage for invalid command":
(https://github.com/bitcoin/bitcoin/pull/26366#discussion_r1281927417)
"remove the check for nullance for command since it's a non-optional field"

while here, maybe test that assertion

```diff
- # check that an invalid command returns an error
+ # check that an invalid or missing command returns an error
assert_raises_rpc_error(-1, 'addnode "node" "command"', self.nodes[0].addnode, node=ip_port, command='abc')
+ assert_raises_rpc_error(-1, 'addnode "node" "command"', self.nodes[0].addnode, node=ip_port)
```

💬 MarcoFalke commented on issue "build: Windows debug cross-build fails with `-O0`":
(https://github.com/bitcoin/bitcoin/issues/28109#issuecomment-1662256481)
Looking at `-ftime-trace=/tmp/`, I don't think there is anything that can be done, other than to nuke boost, or to define a smaller limited interface of our boost usage in a new header and hide the boost includes in the corresponding cpp file?


💬 willcl-ark commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#issuecomment-1662256836)
Concept ACK.

I've not reviewed the code in detail yet, but this is a nice followup to #28060.

It seems to work well so far. I wrote a quick-n-dirty [python script](https://gist.github.com/willcl-ark/d1dd7d80d4581671aa38157960a87ba7) to manually xor my data dir using the same 8 byte key from this pull, and Bitcoin Core at commit fa3c1d230a didn't have any problems reading the xored blocks or key afterwards.

💬 hebasto commented on pull request "[WIP] guix: use `-muse-unaligned-vector-move` for Windows builds":
(https://github.com/bitcoin/bitcoin/pull/28151#issuecomment-1662299643)
Tested Guix build on Windows:

```
C:\Users\hebasto\Desktop\pr28151\bitcoin-6c3f6a9d015c-win64\bitcoin-6c3f6a9d015c>bin\bitcoind.exe -signet
*** stack smashing detected ***: terminated

```

📝 MarcoFalke opened a pull request: "refactor: Remove unused includes from wallet.cpp"
(https://github.com/bitcoin/bitcoin/pull/28200)
This makes compilation of wallet.cpp use a few % less memory and time, locally.

Created in the context of https://github.com/bitcoin/bitcoin/issues/28109, but I don't think it is enough to actually fix this problem.

💬 furszy commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1282008318)
In c0d1110d:

Guess that you wrote this because you saw it on an unit test?

I'm not seeing how this could be possible during the regular node initialization process. We aren't signaling any block prior to initializing the indexes.

💬 theuni commented on pull request "refactor: Remove unused includes from wallet.cpp":
(https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1662346285)
> Good job! The circular dependency "wallet/fees -> wallet/wallet -> wallet/fees" is no longer present.
> Please remove it from EXPECTED_CIRCULAR_DEPENDENCIES in test/lint/lint-circular-dependencies.py
> to make sure this circular dependency is not accidentally reintroduced.

🎉

💬 jonatack commented on pull request "refactor: Remove unused includes from wallet.cpp":
(https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1662369257)
Concept ACK

💬 furszy commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1282048353)
In ba30b60b:

Isn't this conflicting with the `StartBackgroundSync()` call?

Let's say that there are new blocks on the notifications queue at this point. By not setting the `m_synced` flag, we allow `ThreadSync()` to start.
This former one might have some work to do if some of the queued blocks notifications were processed in-between `Init()` and `StartBackgroundSync()`. Then, while this is being done, the validation queue thread might end up setting `m_synced=true`. Which would enable the
...

💬 MarcoFalke commented on pull request "refactor: Remove unused includes from wallet.cpp":
(https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1662427090)
Thanks, force pushed to fix the linter and made the commit hash start with cccc

💬 brandonblack commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#discussion_r1282089319)
Might be better to add a comment above line 29 a la
```
# both nodes disable full-rbf to test BIP125 signaling
```

and then change the comment on line 38 to
```
# second node has default mempool limits
```

🤔 brandonblack reviewed a pull request: "policy: Enable full-rbf by default"
(https://github.com/bitcoin/bitcoin/pull/28132#pullrequestreview-1559299935)
utACK 024403e95d76f824c58851d84468c693bf0cbeae

💬 ryanofsky commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1282083953)
re: https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1282008318

> Guess that you wrote this because you saw it in a unit test?

No, it I believe it happens pretty reliably when you use `-reindex-chainstate`. I saw it reliably when I ran the `test/functional/feature_coinstatsindex.py` "[Test that the index works with -reindex-chainstate](https://github.com/bitcoin/bitcoin/blob/2fa60f0b683cefd7956273986dafe3bde00c98fd/test/functional/feature_coinstatsindex.py#L238-L240)" test and had
...

💬 ryanofsky commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1282110542)
re: https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1282048353

> Isn't this conflicting with the `StartBackgroundSync()` call?
>
> Let's say that there are new blocks on the notifications queue at this point. By not setting the `m_synced` flag, we allow `ThreadSync()` to start.

Yes, that's a good point. I forgot that the m_synced flag is not just used to process notifications, but also to determine whether to start the sync thread.

It's hard to imagine this being a problem
...

💬 Sjors commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1282141206)
Fixed

💬 Sjors commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1662535127)
> Since dumping should be a read-only operation, I think it's better to use the read-only database since it definitely won't write to it like BDB does.

But it could produce a corrupt dump, which someone might rely on for their backup (even if they shouldn't).