💬 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;
```
💬 RicYashiroLee commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1680882053)
Concept ACK - all iniciatives closing the circle on arbitrary data dumping and hash-anchoring to Bitcoin are part of an important step by step process I believe, by now that a lot of vulnerabilities have already been introduced by the BIP/PR mania, and ARE gates for massive exploits we have seen. And with more altcoiners wanting to use Bitcoin, as altcoin casinos destroy their wealth/savings, they still come invariably with their altcoining mindset into Bitcoin also. To have Bitcoin's primary su
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1680882053)
Concept ACK - all iniciatives closing the circle on arbitrary data dumping and hash-anchoring to Bitcoin are part of an important step by step process I believe, by now that a lot of vulnerabilities have already been introduced by the BIP/PR mania, and ARE gates for massive exploits we have seen. And with more altcoiners wanting to use Bitcoin, as altcoin casinos destroy their wealth/savings, they still come invariably with their altcoining mindset into Bitcoin also. To have Bitcoin's primary su
...
💬 theuni commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1296213402)
Actually, looking around, I think it makes more sense to just delete this comment. Autotools checks for a bazillion things it doesn't need. This is just one of those quirks that can't be turned off.
(The issue is that LLVM's CMake file checks for terminfo even though we don't need it. It doesn't seem to be a problem in the real world. If we encounter an actual issue, THEN we can worry about working around it.)
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1296213402)
Actually, looking around, I think it makes more sense to just delete this comment. Autotools checks for a bazillion things it doesn't need. This is just one of those quirks that can't be turned off.
(The issue is that LLVM's CMake file checks for terminfo even though we don't need it. It doesn't seem to be a problem in the real world. If we encounter an actual issue, THEN we can worry about working around it.)
💬 ryanofsky commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1296088099)
In commit "assumeutxo: disallow -reindex when background syncing" (3b9d3d63d9ccb609ec08da34776f7c5f29d21a20)
re: https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1291426426
I don't understand the advice to "wait for background sync to complete" in this error message. I thought use-cases for reindex were mostly things like recovering from data corruption or restoring from backup. Is there a case where you would ever want to wait for a sync to complete and then reindex after that?
...
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1296088099)
In commit "assumeutxo: disallow -reindex when background syncing" (3b9d3d63d9ccb609ec08da34776f7c5f29d21a20)
re: https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1291426426
I don't understand the advice to "wait for background sync to complete" in this error message. I thought use-cases for reindex were mostly things like recovering from data corruption or restoring from backup. Is there a case where you would ever want to wait for a sync to complete and then reindex after that?
...
💬 ryanofsky commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1296254459)
In commit "validation: MaybeRebalanceCaches when chain leaves IBD" (5ea9abfbac3543f2d2cef90a1528abed35623822)
It seems dangerous if just calling IsInitialBlockDownload to find out IBD state could now block and resize caches and flush state to disk. It also makes the codebase hard to understand if you can't answer a basic question like "when do caches get resized" without having to look up IsInitialBlockDownload() calls all over the codebase.
I think it would be better to choose one place w
...
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1296254459)
In commit "validation: MaybeRebalanceCaches when chain leaves IBD" (5ea9abfbac3543f2d2cef90a1528abed35623822)
It seems dangerous if just calling IsInitialBlockDownload to find out IBD state could now block and resize caches and flush state to disk. It also makes the codebase hard to understand if you can't answer a basic question like "when do caches get resized" without having to look up IsInitialBlockDownload() calls all over the codebase.
I think it would be better to choose one place w
...
💬 murchandamus commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#issuecomment-1681070555)
My fuzzer has performed over 10M runs without uncovering an issue.
ACK https://github.com/bitcoin/bitcoin/commit/d501e1a2283d82b833260bf3372512d616a62a4c
(https://github.com/bitcoin/bitcoin/pull/27585#issuecomment-1681070555)
My fuzzer has performed over 10M runs without uncovering an issue.
ACK https://github.com/bitcoin/bitcoin/commit/d501e1a2283d82b833260bf3372512d616a62a4c
🤔 glozow reviewed a pull request: "p2p: Drop m_recently_announced_invs bloom filter"
(https://github.com/bitcoin/bitcoin/pull/27675#pullrequestreview-1581283327)
reACK fb02ba3c5f5bcd96b5e3622ef001b8e57ce63fc0
(https://github.com/bitcoin/bitcoin/pull/27675#pullrequestreview-1581283327)
reACK fb02ba3c5f5bcd96b5e3622ef001b8e57ce63fc0
💬 murchandamus commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1296358180)
@josibake: It’s rather that bech32(m) addresses are the odd ones out. Before native segwit outputs all addresses across testnet and regtest were the same. After rereading #12314, I would still recommend that you use the same hrp for all test-networks.
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1296358180)
@josibake: It’s rather that bech32(m) addresses are the odd ones out. Before native segwit outputs all addresses across testnet and regtest were the same. After rereading #12314, I would still recommend that you use the same hrp for all test-networks.
🤔 murchandamus reviewed a pull request: "Break up script/standard.{h/cpp}"
(https://github.com/bitcoin/bitcoin/pull/28244#pullrequestreview-1581332473)
ACK 91d924ede1b421df31c895f4f43359e453a09ca5
(https://github.com/bitcoin/bitcoin/pull/28244#pullrequestreview-1581332473)
ACK 91d924ede1b421df31c895f4f43359e453a09ca5