💬 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):
```
(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():
```
(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.
(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
...
(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
(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?
(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
(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
(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
(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.
(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
...
(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.
(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).
(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).
💬 hebasto commented on pull request "test: Treat `bitcoin-wallet` binary in the same way as others":
(https://github.com/bitcoin/bitcoin/pull/27554#discussion_r1186040867)
Adjusted.
(https://github.com/bitcoin/bitcoin/pull/27554#discussion_r1186040867)
Adjusted.
⚠️ fanquake opened an issue: "TSAN: lock-order-inversion (potential deadlock) in ZapSelectTx test"
(https://github.com/bitcoin/bitcoin/issues/27582)
master (6c7ebcc14b7908a67a8f8764b398e76c8fb4fe8b) running the TSAN CI job, on Fedora 38, with podman:
```bash
==================
WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=94411)
Cycle in lock order graph: M0 (0xffff8c427208) => M1 (0xffff8c42d868) => M2 (0xffff8c42da18) => M0
Mutex M1 acquired here while holding mutex M0 in main thread:
#0 pthread_rwlock_wrlock <null> (test_bitcoin+0x150920) (BuildId: e6d773dbb6475135a1747fe19e2eb18593ee1a4b)
#
...
(https://github.com/bitcoin/bitcoin/issues/27582)
master (6c7ebcc14b7908a67a8f8764b398e76c8fb4fe8b) running the TSAN CI job, on Fedora 38, with podman:
```bash
==================
WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=94411)
Cycle in lock order graph: M0 (0xffff8c427208) => M1 (0xffff8c42d868) => M2 (0xffff8c42da18) => M0
Mutex M1 acquired here while holding mutex M0 in main thread:
#0 pthread_rwlock_wrlock <null> (test_bitcoin+0x150920) (BuildId: e6d773dbb6475135a1747fe19e2eb18593ee1a4b)
#
...
💬 glozow commented on pull request "Allow accepting non-standard transactions on mainnet":
(https://github.com/bitcoin/bitcoin/pull/27578#issuecomment-1536207253)
Concept NACK. These checks exist for very good reasons; being able to bypass them on mainnet is a huge footgun. If there is a specific standardness rule that you can show (1) provides little utility to the node or network and (2) is getting in the way of a use case, we can discuss changing that rule.
> There are many projects today that have part of their workflow "email a miner this transaction" (inscriptions, https://github.com/supertestnet/breaker-of-jpegs, etc). This is a problem and will
...
(https://github.com/bitcoin/bitcoin/pull/27578#issuecomment-1536207253)
Concept NACK. These checks exist for very good reasons; being able to bypass them on mainnet is a huge footgun. If there is a specific standardness rule that you can show (1) provides little utility to the node or network and (2) is getting in the way of a use case, we can discuss changing that rule.
> There are many projects today that have part of their workflow "email a miner this transaction" (inscriptions, https://github.com/supertestnet/breaker-of-jpegs, etc). This is a problem and will
...
💬 MarcoFalke commented on pull request "test, init: perturb file to ensure failure instead of only deleting them":
(https://github.com/bitcoin/bitcoin/pull/26653#issuecomment-1536207299)
lgtm ACK c371cae07a7ba045130568b6abc470eaa4f95ef4
(https://github.com/bitcoin/bitcoin/pull/26653#issuecomment-1536207299)
lgtm ACK c371cae07a7ba045130568b6abc470eaa4f95ef4
💬 MarcoFalke commented on issue "TSAN: lock-order-inversion (potential deadlock) in ZapSelectTx test":
(https://github.com/bitcoin/bitcoin/issues/27582#issuecomment-1536209476)
Is this reproducible?
(https://github.com/bitcoin/bitcoin/issues/27582#issuecomment-1536209476)
Is this reproducible?
💬 jamesob commented on pull request "assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/15606#issuecomment-1536222186)
Thanks for testing!
> except that my mempool was empty until I stopped and restarted bitcoind?
Interesting, I'll look at this today; probably some part of net_processing that's looking at `chainman.IsAnyIBD()` when it should be using `ActiveChainstate().IsInitialBlockDownload()`.
(https://github.com/bitcoin/bitcoin/pull/15606#issuecomment-1536222186)
Thanks for testing!
> except that my mempool was empty until I stopped and restarted bitcoind?
Interesting, I'll look at this today; probably some part of net_processing that's looking at `chainman.IsAnyIBD()` when it should be using `ActiveChainstate().IsInitialBlockDownload()`.
💬 furszy commented on pull request "net: call getaddrinfo() in detachable thread to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/27557#discussion_r1186063927)
> Afaict this won't work here, because the thread might not finish and then the pool would just get clogged (see comment about abandoning the thread).
k, `getaddrinfo` doesn't have a timeout and `getaddrinfo_a` seems to have a segfault. Cool..
> Performance should also not really matter here since this doesn't happen that often and startup time for a thread vs. time of a DNS lookup is probably negligible.
I was thinking more about context switching. Isn't this used for `OpenNetworkConne
...
(https://github.com/bitcoin/bitcoin/pull/27557#discussion_r1186063927)
> Afaict this won't work here, because the thread might not finish and then the pool would just get clogged (see comment about abandoning the thread).
k, `getaddrinfo` doesn't have a timeout and `getaddrinfo_a` seems to have a segfault. Cool..
> Performance should also not really matter here since this doesn't happen that often and startup time for a thread vs. time of a DNS lookup is probably negligible.
I was thinking more about context switching. Isn't this used for `OpenNetworkConne
...
💬 sipa commented on issue "[Linux] Add wayland support":
(https://github.com/bitcoin/bitcoin/issues/19950#issuecomment-1536232647)
Is it the case that our `bitcion-qt` release binaries don't work on Wayland at all? If so, that should be fixed, I think. Many distributions are increasingly migrating away from X.
(https://github.com/bitcoin/bitcoin/issues/19950#issuecomment-1536232647)
Is it the case that our `bitcion-qt` release binaries don't work on Wayland at all? If so, that should be fixed, I think. Many distributions are increasingly migrating away from X.