💬 Sjors commented on pull request "Multiprocess bitcoin":
(https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-1536075360)
I was able to reproduce the crash I found [here](https://github.com/bitcoin/bitcoin/pull/19461#issuecomment-1536056897) using only this PR.

Details: running Ubuntu 23.04, libmultiprocess at fc28a48f01af9730be3b49585e718e11c5eea0c5 and capnproto 0.10.4 compiled from source (because of https://github.com/chaincodelabs/libmultiprocess/issues/68#issuecomment-1527856219).

```
./configure --enable-werror --enable-suppress-external-warnings --without-gui --enable-multiprocess
…
src/bitcoin-no
...

💬 fanquake commented on pull request "assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/15606#issuecomment-1536093556)
> Known weird behavior

Runing through your steps above, everything *seems* to be working, except that my mempool was empty until I stopped and restarted bitcoind?

> If you restart the node in the middle of the bg sync, nChainTx for the snapshot chain will not properly repopulate and your progress= for the snapshot chainstate will be weird. I think there's an easy fix for this, which I'm investigating.

Yea. After a restart:
```bash
# ./src/bitcoin-cli -datadir=${AU_DATADIR} getblockcha
...

👍 TheCharlatan approved a pull request: "refactor: Remove need to pass chainparams to BlockManager methods"
(https://github.com/bitcoin/bitcoin/pull/27570#pullrequestreview-1414604996)
ACK fa5d7c39eb992467e43b12db213b27913374fb83

Thank you for following up so quickly!

💬 TheCharlatan commented on pull request "refactor: Remove need to pass chainparams to BlockManager methods":
(https://github.com/bitcoin/bitcoin/pull/27570#discussion_r1185976774)
Nit: Can be `const` (here and elsewhere) and skip the `node::` namespacing, since it is imported above. If this is merged before #27125 I will follow-up there.

💬 MarcoFalke commented on pull request "fuzz: BIP 42, BIP 30, CVE-2018-17144":
(https://github.com/bitcoin/bitcoin/pull/17860#discussion_r1185989416)
Nice. I get "number go down" from 94k to 90k in the last force push with steps to reproduce:


```
FUZZ=utxo_total_supply /usr/bin/time --f='%M' ./src/test/fuzz/fuzz -runs=1 -- --printtoconsole=0

💬 MarcoFalke commented on pull request "fuzz: BIP 42, BIP 30, CVE-2018-17144":
(https://github.com/bitcoin/bitcoin/pull/17860#discussion_r1185989734)
thx, done

👍 stickies-v approved a pull request: "test: Treat `bitcoin-wallet` binary in the same way as others"
(https://github.com/bitcoin/bitcoin/pull/27554#pullrequestreview-1414615945)
ACK ef01337bbb3d5269db6a26e42c7bbd238000ce2f

Nice little refactoring (modulo being able to now set `BITCOINWALLET` which is new behaviour), less code duplication and functions with smaller scope.

💬 stickies-v commented on pull request "test: Treat `bitcoin-wallet` binary in the same way as others":
(https://github.com/bitcoin/bitcoin/pull/27554#discussion_r1185989630)
nit
```suggestion
"""Update self.options with the paths of all binaries from environment variables or their default values"""
def set_binary_paths(self):
```

💬 stickies-v commented on pull request "test: Treat `bitcoin-wallet` binary in the same way as others":
(https://github.com/bitcoin/bitcoin/pull/27554#discussion_r1185984288)
nit
```suggestion
for binary, [attribute_name, env_variable_name] in binaries.items():
```

💬 MarcoFalke commented on pull request "refactor: Remove need to pass chainparams to BlockManager methods":
(https://github.com/bitcoin/bitcoin/pull/27570#discussion_r1185993524)
I left it mutable to make it less code-churn to run `ApplyArgsManOptions` in the future, which you called in 27125 at some point, but no longer in the current state.

I'll remove `node::` in the next push or maybe a follow-up.

💬 Sjors commented on pull request "multiprocess: Add bitcoin-gui -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/19461#issuecomment-1536135020)
I'm able to produce another crash that may be related to this PR. When using an external signer that's connected to the Ubuntu machine, and trying to spend with it from the macOs GUI. After I click Sign on device, and hit "send":

```
Assertion failed: (!complete), function sendButtonClicked, file sendcoinsdialog.cpp, line 525.
zsh: abort src/bitcoin-gui -ipcconnect=unix:///tmp/remote-node.sock
```

It also crashes if I create a PSBT (options -> wallet -> enable psbt controls) instea
...

💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1185099728)
Passing a pointer from one process to another one is UB. I wonder if there should be a compile failure if someone adds a pointer type to the multiprocess interface? cc ryan

💬 fanquake commented on issue "ci: failure in Docker build step":
(https://github.com/bitcoin/bitcoin/issues/27492#issuecomment-1536138989)
I think "rebase if this is seen" is an ok solution here. Should we close for now?

👍 theStack approved a pull request: "p2p, rpc, test: `getpeerinfo` improv + add test coverage for invalid command"
(https://github.com/bitcoin/bitcoin/pull/26366#pullrequestreview-1414640206)
Code-review ACK 40660d517022b30e3ab4dcf852a6c86502fa97f3

💬 LarryRuane commented on pull request "util: improve FindByte() performance":
(https://github.com/bitcoin/bitcoin/pull/19690#issuecomment-1536159645)
Force pushed rebase to fix hidden merge conflict, thanks @achow101

💬 hebasto commented on pull request "test: Treat `bitcoin-wallet` binary in the same way as others":
(https://github.com/bitcoin/bitcoin/pull/27554#issuecomment-1536159697)
Updated ef01337bbb3d5269db6a26e42c7bbd238000ce2f -> de8b7ab2e023e28168e870629c2423dab589085e ([pr27554.03](https://github.com/hebasto/bitcoin/commits/pr27554.03) -> [pr27554.04](https://github.com/hebasto/bitcoin/commits/pr27554.04), [diff](https://github.com/hebasto/bitcoin/compare/pr27554.03..pr27554.04)):

- addressed @stickies-v's comments

💬 stickies-v commented on pull request "test: Treat `bitcoin-wallet` binary in the same way as others":
(https://github.com/bitcoin/bitcoin/pull/27554#discussion_r1186013450)
Oops sorry, the docstring is meant to go underneath the function `def`, not above. My bad.

💬 sdaftuar commented on pull request "Allow accepting non-standard transactions on mainnet":
(https://github.com/bitcoin/bitcoin/pull/27578#issuecomment-1536164061)
> If people want to accept non standard transactions, we should let them.

The problem is that miners need DoS protection too. So I don't understand what you think we accomplish by giving everyone a knob to turn off DoS protections -- under what circumstances is that safe to use? If the argument is that just some miners are getting access to high feerate transactions, and we need the network to relay those transactions more broadly so that mining is competitive, then we need a way to do so t
...

📝 fjahr opened a pull request: "net: Continuous ASMap health check"
(https://github.com/bitcoin/bitcoin/pull/27581)
There are certain statistics we can collect by running all our known clearnet addresses against the ASMap file. This could show issues with a maliciously manipulated file or with an old file that has decayed with time.

This is just a proof of concept for now. My idea currently is to run the analysis once per day and print the results to logs if an ASMap file is used.

💬 benthecarman commented on pull request "Allow accepting non-standard transactions on mainnet":
(https://github.com/bitcoin/bitcoin/pull/27578#issuecomment-1536189204)
The problem is that standardness is an overloaded term, it contains some DoS protection while also just disallowing other things. If it is that dangerous, proper warnings can be added, however, DoS attacks are already happening on bitcoin that are within standardness rules (stamps).