💬 Sjors commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295649173)
384c798cc3836558463e88ec7f563b236f50bf22 could we annotate the tuple?
```cpp
std::tuple<Span<const uint8_t> /*to_send*/, bool /*more*/, const std::string& /* m_type */>
```
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295649173)
384c798cc3836558463e88ec7f563b236f50bf22 could we annotate the tuple?
```cpp
std::tuple<Span<const uint8_t> /*to_send*/, bool /*more*/, const std::string& /* m_type */>
```
💬 brunoerg commented on pull request "fuzz: call `LookupSubNet` before calling `Ban` with a subnet":
(https://github.com/bitcoin/bitcoin/pull/27935#issuecomment-1680589999)
Rebased.
(https://github.com/bitcoin/bitcoin/pull/27935#issuecomment-1680589999)
Rebased.
💬 vasild commented on pull request "Make all networking code mockable":
(https://github.com/bitcoin/bitcoin/pull/21878#issuecomment-1680606505)
`b497200c7a...80d9dfd0f0`: rebase due to conflicts
(https://github.com/bitcoin/bitcoin/pull/21878#issuecomment-1680606505)
`b497200c7a...80d9dfd0f0`: rebase due to conflicts
👍 hebasto approved a pull request: "ci: Refactor: Remove CI_USE_APT_INSTALL"
(https://github.com/bitcoin/bitcoin/pull/28278#pullrequestreview-1580616607)
ACK fa04623bbb284b35b27b2575b217341ab2e81e48.
Thank you!
(https://github.com/bitcoin/bitcoin/pull/28278#pullrequestreview-1580616607)
ACK fa04623bbb284b35b27b2575b217341ab2e81e48.
Thank you!
💬 vasild commented on pull request "Make it possible to disable Tor binds and abort startup on bind failure":
(https://github.com/bitcoin/bitcoin/pull/22729#issuecomment-1680635537)
`7180859e7e...6a51eaf4a9`: rebase, did not lose any relevance through time
An alternative to this is described in:
https://github.com/bitcoin/bitcoin/pull/22729#issuecomment-1313383147
that would be more complicated, but maybe it would be more appealing to reviewers?
> only concern is if there's a way around https://github.com/bitcoin/bitcoin/pull/22729#issuecomment-900506737 - the need for =onion to accept inbound connections wouldn't be intuitive to users and might break existing confi
...
(https://github.com/bitcoin/bitcoin/pull/22729#issuecomment-1680635537)
`7180859e7e...6a51eaf4a9`: rebase, did not lose any relevance through time
An alternative to this is described in:
https://github.com/bitcoin/bitcoin/pull/22729#issuecomment-1313383147
that would be more complicated, but maybe it would be more appealing to reviewers?
> only concern is if there's a way around https://github.com/bitcoin/bitcoin/pull/22729#issuecomment-900506737 - the need for =onion to accept inbound connections wouldn't be intuitive to users and might break existing confi
...
💬 theuni commented on pull request "refactor: Enforce C-str fmt strings in WalletLogPrintf()":
(https://github.com/bitcoin/bitcoin/pull/28237#discussion_r1295934037)
Huh. For whatever reason, my clang-tidy-16 acts as if `IgnoreUnlessSpelledInSource` is set. That's weird.
It came from here: https://github.com/llvm/llvm-project/releases/download/llvmorg-16.0.0/clang+llvm-16.0.0-x86_64-linux-gnu-ubuntu-18.04.tar.xz
I can't reproduce with clang-tidy-15. clang-query-16 only matches if traversal is set to `IgnoreUnlessSpelledInSource`.
> $ /opt/clang+llvm-15.0.2-x86_64-unknown-linux-gnu/bin/clang-query wallet/wallet.cpp -- -DHAVE_BUILD_INFO -I. -I./minisk
...
(https://github.com/bitcoin/bitcoin/pull/28237#discussion_r1295934037)
Huh. For whatever reason, my clang-tidy-16 acts as if `IgnoreUnlessSpelledInSource` is set. That's weird.
It came from here: https://github.com/llvm/llvm-project/releases/download/llvmorg-16.0.0/clang+llvm-16.0.0-x86_64-linux-gnu-ubuntu-18.04.tar.xz
I can't reproduce with clang-tidy-15. clang-query-16 only matches if traversal is set to `IgnoreUnlessSpelledInSource`.
> $ /opt/clang+llvm-15.0.2-x86_64-unknown-linux-gnu/bin/clang-query wallet/wallet.cpp -- -DHAVE_BUILD_INFO -I. -I./minisk
...
💬 theuni commented on pull request "refactor: Enforce C-str fmt strings in WalletLogPrintf()":
(https://github.com/bitcoin/bitcoin/pull/28237#issuecomment-1680641624)
ACK fa60fa3b0cba4a30726af8e0e9d1e84e14849eda
(https://github.com/bitcoin/bitcoin/pull/28237#issuecomment-1680641624)
ACK fa60fa3b0cba4a30726af8e0e9d1e84e14849eda
💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1295951009)
This is still up-for-grabs, if someone wants to take it :)
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1295951009)
This is still up-for-grabs, if someone wants to take it :)
💬 vasild commented on pull request "doc: clarify that LOCK() does AssertLockNotHeld() internally":
(https://github.com/bitcoin/bitcoin/pull/27116#issuecomment-1680659641)
1. Should I change `LOCK()` to print everything to `stderr`, in addition to the log? That is, print this to `stderr` as well:
```
2023-02-17T11:47:18Z DOUBLE LOCK DETECTED
2023-02-17T11:47:18Z Lock order:
2023-02-17T11:47:18Z (*) 'flock' in net.cpp:1404 (in thread 'dnsseed')
2023-02-17T11:47:18Z (*) 'flock' in net.cpp:1399 (in thread 'dnsseed')
```
2. Or do people prefer to write both `AssertLockNotHeld()` and `LOCK()`, like this:
```cpp
void f()
{
AssertLockNotHeld(mutex)
...
(https://github.com/bitcoin/bitcoin/pull/27116#issuecomment-1680659641)
1. Should I change `LOCK()` to print everything to `stderr`, in addition to the log? That is, print this to `stderr` as well:
```
2023-02-17T11:47:18Z DOUBLE LOCK DETECTED
2023-02-17T11:47:18Z Lock order:
2023-02-17T11:47:18Z (*) 'flock' in net.cpp:1404 (in thread 'dnsseed')
2023-02-17T11:47:18Z (*) 'flock' in net.cpp:1399 (in thread 'dnsseed')
```
2. Or do people prefer to write both `AssertLockNotHeld()` and `LOCK()`, like this:
```cpp
void f()
{
AssertLockNotHeld(mutex)
...
📝 MarcoFalke opened a pull request: "ci: Add test-each-commit task"
(https://github.com/bitcoin/bitcoin/pull/28279)
Currently, if a pull request has more than one commit, previous commits may fail to compile, or may fail the tests. This is problematic, because it breaks git-bisect, or worse.
Fix this by adding a CI task for this.
(https://github.com/bitcoin/bitcoin/pull/28279)
Currently, if a pull request has more than one commit, previous commits may fail to compile, or may fail the tests. This is problematic, because it breaks git-bisect, or worse.
Fix this by adding a CI task for this.
💬 petertodd commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1680748587)
@luke-jr
> I'm not aware of any legitimate usage of bare multisig, so this should be a costless way to filter more spam. Knots has had this policy for years.
OpenTimestamps has previously used bare multisig for timestamp transactions, as it's a bit more efficient than OP_Return. The current OpenTimestamps Server does not, as when I wrote it I was expecting bare multisig to be made non-standard. But as that didn't happen, I should look into adding it again.
Concept NACK without another j
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1680748587)
@luke-jr
> I'm not aware of any legitimate usage of bare multisig, so this should be a costless way to filter more spam. Knots has had this policy for years.
OpenTimestamps has previously used bare multisig for timestamp transactions, as it's a bit more efficient than OP_Return. The current OpenTimestamps Server does not, as when I wrote it I was expecting bare multisig to be made non-standard. But as that didn't happen, I should look into adding it again.
Concept NACK without another j
...
💬 BrandonOdiwuor commented on pull request "Silent Payments: receiving":
(https://github.com/bitcoin/bitcoin/pull/28202#issuecomment-1680760109)
Tested ACK with some comments at the end
Was able to successfully run unit and functional tests on 'pr/28202' branch and verified that the tests fail against the master branch
Manually tested on regtest as described below:
```sh
$ ./src/bitcoin-cli -regtest -named createwallet wallet_name="sp-wallet" silent_payment=true
```
```sh
$ ./src/bitcoin-cli -regtest getwalletinfo # showed that a wallet with silent payment enabled was created successfully
{
"walletname": "sp-wallet",
...
(https://github.com/bitcoin/bitcoin/pull/28202#issuecomment-1680760109)
Tested ACK with some comments at the end
Was able to successfully run unit and functional tests on 'pr/28202' branch and verified that the tests fail against the master branch
Manually tested on regtest as described below:
```sh
$ ./src/bitcoin-cli -regtest -named createwallet wallet_name="sp-wallet" silent_payment=true
```
```sh
$ ./src/bitcoin-cli -regtest getwalletinfo # showed that a wallet with silent payment enabled was created successfully
{
"walletname": "sp-wallet",
...
💬 petertodd commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1680760708)
@mikeinspace
> We will go straight to mining pools if we have to (who LOVE to include our high fee transactions).
Relevant to this discussion and OP_Return: are there already agreements with mining pools in your community to get non-std txs mined on a *regular* basis? Eg do any run peerable nodes with non-std-accepting patches?
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1680760708)
@mikeinspace
> We will go straight to mining pools if we have to (who LOVE to include our high fee transactions).
Relevant to this discussion and OP_Return: are there already agreements with mining pools in your community to get non-std txs mined on a *regular* basis? Eg do any run peerable nodes with non-std-accepting patches?
🤔 BrandonOdiwuor reviewed a pull request: "Silent Payments: receiving"
(https://github.com/bitcoin/bitcoin/pull/28202#pullrequestreview-1580831598)
Tested ACK with some comments at the end
Was able to successfully run unit and functional tests on 'pr/28202' branch and verified that the tests fail against the master branch
Manually tested on regtest as described below:
```sh
$ ./src/bitcoin-cli -regtest -named createwallet wallet_name="sp-wallet" silent_payment=true
```
```sh
$ ./src/bitcoin-cli -regtest getwalletinfo # showed that a wallet with silent payment enabled was created successfully
{
"walletname": "sp-wallet",
...
(https://github.com/bitcoin/bitcoin/pull/28202#pullrequestreview-1580831598)
Tested ACK with some comments at the end
Was able to successfully run unit and functional tests on 'pr/28202' branch and verified that the tests fail against the master branch
Manually tested on regtest as described below:
```sh
$ ./src/bitcoin-cli -regtest -named createwallet wallet_name="sp-wallet" silent_payment=true
```
```sh
$ ./src/bitcoin-cli -regtest getwalletinfo # showed that a wallet with silent payment enabled was created successfully
{
"walletname": "sp-wallet",
...
💬 furszy commented on pull request "test: display abrupt shutdown errors in console output":
(https://github.com/bitcoin/bitcoin/pull/28253#discussion_r1296039055)
> > In a first glance, that doesn't seems to print the nodes' stderr. That seems to be related to the job stderr (the test run in a subprocess). And those aren't connected.
>
> Currently, you are reading the node's stderr and the printing it to stdout, so it _should_ appear as `print(BOLD[1] + 'stdout:\n' + BOLD[0] + stdout + '\n')`.
>
> > Ok. The result is pretty much the same. Just the stack trace is a bit longer.
>
> The difference should be that the output won't be shown in the CI t
...
(https://github.com/bitcoin/bitcoin/pull/28253#discussion_r1296039055)
> > In a first glance, that doesn't seems to print the nodes' stderr. That seems to be related to the job stderr (the test run in a subprocess). And those aren't connected.
>
> Currently, you are reading the node's stderr and the printing it to stdout, so it _should_ appear as `print(BOLD[1] + 'stdout:\n' + BOLD[0] + stdout + '\n')`.
>
> > Ok. The result is pretty much the same. Just the stack trace is a bit longer.
>
> The difference should be that the output won't be shown in the CI t
...
🤔 murchandamus reviewed a pull request: "fuzz: improve `coinselection`"
(https://github.com/bitcoin/bitcoin/pull/27585#pullrequestreview-1580918089)
Code changes look good to me, crACK d501e1a2283d82b833260bf3372512d616a62a4c
Started some fuzzing, will let you know if it finds anything.
(https://github.com/bitcoin/bitcoin/pull/27585#pullrequestreview-1580918089)
Code changes look good to me, crACK d501e1a2283d82b833260bf3372512d616a62a4c
Started some fuzzing, will let you know if it finds anything.
💬 vasild commented on pull request "p2p: bugfixes, logic and logging improvements":
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1296069949)
Before this it would compare just the address and after this change it would compare the address and the port. I think that is undesirable because for inbound connections we see the peer as e.g. `1.2.3.4:somerandomport` and when we try to connect to `1.2.3.4:8333` we should assume we are already connected to that peer.
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1296069949)
Before this it would compare just the address and after this change it would compare the address and the port. I think that is undesirable because for inbound connections we see the peer as e.g. `1.2.3.4:somerandomport` and when we try to connect to `1.2.3.4:8333` we should assume we are already connected to that peer.
💬 vasild commented on pull request "p2p: bugfixes, logic and logging improvements":
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1296121513)
Why not use `LogPrintLevel(BCLog::NET, BCLog::Level::Debug`?
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1296121513)
Why not use `LogPrintLevel(BCLog::NET, BCLog::Level::Debug`?
💬 vasild commented on pull request "p2p: bugfixes, logic and logging improvements":
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1296074743)
The existent code in `master` seems to be comparing the ports. Is there another bug that an inbound connection from `1.2.3.4` would not be considered as "we are connected to `1.2.3.4:8333`"?
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1296074743)
The existent code in `master` seems to be comparing the ports. Is there another bug that an inbound connection from `1.2.3.4` would not be considered as "we are connected to `1.2.3.4:8333`"?
💬 vasild commented on pull request "p2p: bugfixes, logic and logging improvements":
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1296106505)
Or this:
```cpp
return m_added_nodes.insert(strNode).second;
```
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1296106505)
Or this:
```cpp
return m_added_nodes.insert(strNode).second;
```