💬 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
🤔 jonatack reviewed a pull request: "test: check backup from `migratewallet` can be successfully restored"
(https://github.com/bitcoin/bitcoin/pull/28257#pullrequestreview-1581364835)
Post-merge ACK.
For the getwalletinfo['balance'] check, RPC getbalances['mine']['trusted'] should work as well.
(https://github.com/bitcoin/bitcoin/pull/28257#pullrequestreview-1581364835)
Post-merge ACK.
For the getwalletinfo['balance'] check, RPC getbalances['mine']['trusted'] should work as well.
💬 jonatack commented on pull request "p2p: bugfixes, logic and logging improvements":
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1296393528)
Thank you, I recalled there was something like that. Will do.
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1296393528)
Thank you, I recalled there was something like that. Will do.
🤔 mzumsande reviewed a pull request: "p2p: adaptive connections services flags"
(https://github.com/bitcoin/bitcoin/pull/28170#pullrequestreview-1581277414)
Concept ACK
This seems to be similar to the suggestion by gmaxwell in https://github.com/bitcoin/bitcoin/pull/10387#issuecomment-343357330
It also is a good thing to have `protocol.h` not depend on dynamic `net_processing` state.
(https://github.com/bitcoin/bitcoin/pull/28170#pullrequestreview-1581277414)
Concept ACK
This seems to be similar to the suggestion by gmaxwell in https://github.com/bitcoin/bitcoin/pull/10387#issuecomment-343357330
It also is a good thing to have `protocol.h` not depend on dynamic `net_processing` state.
💬 mzumsande commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1296335745)
The comment ("shortcut for...") was helpful for me, any reason to drop it?
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1296335745)
The comment ("shortcut for...") was helpful for me, any reason to drop it?
💬 mzumsande commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1296375222)
`m_initial_sync_finished` is set using AdjustedTime (see `CanDirectFetch()`) while it's unset using local time. In spite of plans to get rid of AdjustedTime, I think it would be better to use it here as well, otherwise there could be constant switching back and forth in some situations where the two times differ.
Also, why introduce a new magic number of 290, can't we just use `NODE_NETWORK_LIMITED_MIN_BLOCKS` (maybe + 2, if there is a reason for it)?
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1296375222)
`m_initial_sync_finished` is set using AdjustedTime (see `CanDirectFetch()`) while it's unset using local time. In spite of plans to get rid of AdjustedTime, I think it would be better to use it here as well, otherwise there could be constant switching back and forth in some situations where the two times differ.
Also, why introduce a new magic number of 290, can't we just use `NODE_NETWORK_LIMITED_MIN_BLOCKS` (maybe + 2, if there is a reason for it)?
💬 mzumsande commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1296355296)
Why stop doing extra block-relay peers in this situation? These aren't attempted during original IBD because we are expected to be behind the tip. However, in this super-stale situation which should really not happen normally, it seems that they could only help?
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1296355296)
Why stop doing extra block-relay peers in this situation? These aren't attempted during original IBD because we are expected to be behind the tip. However, in this super-stale situation which should really not happen normally, it seems that they could only help?
💬 jonatack commented on pull request "p2p: bugfixes, logic and logging improvements":
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1296405960)
I didn't change the level from the current `LogPrintf` since 236618061a445d2cb11e722cfac5fdae5be26abb in 2017. Since this shouldn't normally occur, unconditionally logging seems helpful.
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1296405960)
I didn't change the level from the current `LogPrintf` since 236618061a445d2cb11e722cfac5fdae5be26abb in 2017. Since this shouldn't normally occur, unconditionally logging seems helpful.
💬 russeree commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1681249694)
Another vector that could be used to circumvent restrictions on p2ms or even p2pk relaying would be to use p2wsh. Using a p2wsh tx outpoint would allow users to embed up to 256 bits (_32 bytes_) of arbitrary data per. These outpoints would also be unprunable.
The usable data storage capabilities of p2wsh are equal to p2pk with 32 bytes per outpoint. This would increase the number of required outpoints by 3x compared to p2ms.
The existence of p2wsh in this context makes restricting p2pk
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1681249694)
Another vector that could be used to circumvent restrictions on p2ms or even p2pk relaying would be to use p2wsh. Using a p2wsh tx outpoint would allow users to embed up to 256 bits (_32 bytes_) of arbitrary data per. These outpoints would also be unprunable.
The usable data storage capabilities of p2wsh are equal to p2pk with 32 bytes per outpoint. This would increase the number of required outpoints by 3x compared to p2ms.
The existence of p2wsh in this context makes restricting p2pk
...
💬 jonatack commented on pull request "p2p: bugfixes, logic and logging improvements":
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1296427115)
Yes, that is currently an issue as well that I have noticed. See also https://github.com/bitcoin/bitcoin/pull/28155 (which I plan to review).
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1296427115)
Yes, that is currently an issue as well that I have noticed. See also https://github.com/bitcoin/bitcoin/pull/28155 (which I plan to review).
💬 LarryRuane commented on pull request "test: rpc: add last block announcement time to getpeerinfo result":
(https://github.com/bitcoin/bitcoin/pull/27052#issuecomment-1681276711)
Force-pushed to implement review suggestion to use `TicksSinceEpoch()` (thanks!)
(https://github.com/bitcoin/bitcoin/pull/27052#issuecomment-1681276711)
Force-pushed to implement review suggestion to use `TicksSinceEpoch()` (thanks!)
💬 Retropex commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1681346103)
@Russeree you seem obstinate in believing that I'm trying to force things, that's not the case.
I am also not looking for the consensus that will come naturally or not depending on the relevance of the changes.
I just want to have tools to give nodes runner the choice. #27589
Unlike @petertodd who tried to remove a tool for nodes runner #28130, I want others to be implemented.
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1681346103)
@Russeree you seem obstinate in believing that I'm trying to force things, that's not the case.
I am also not looking for the consensus that will come naturally or not depending on the relevance of the changes.
I just want to have tools to give nodes runner the choice. #27589
Unlike @petertodd who tried to remove a tool for nodes runner #28130, I want others to be implemented.
📝 andrewtoth opened a pull request: "Don't empty dbcache on prune flushes: >30% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/28280)
Since https://github.com/bitcoin/bitcoin/pull/17487 we no longer need to clear the coins cache when syncing to disk. A warm coins cache significantly speeds up block connection, and only needs to be fully flushed when nearing the `dbcache` limit.
For frequent pruning flushes there's no need to empty the cache and kill connect block speed. However, simply using `Sync` in place of `Flush` actually slows down a pruned full IBD with a high `dbcache` value. This is because as the cache grows, sync
...
(https://github.com/bitcoin/bitcoin/pull/28280)
Since https://github.com/bitcoin/bitcoin/pull/17487 we no longer need to clear the coins cache when syncing to disk. A warm coins cache significantly speeds up block connection, and only needs to be fully flushed when nearing the `dbcache` limit.
For frequent pruning flushes there's no need to empty the cache and kill connect block speed. However, simply using `Sync` in place of `Flush` actually slows down a pruned full IBD with a high `dbcache` value. This is because as the cache grows, sync
...