💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295209819)
I've added a comment to explain this better.
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295209819)
I've added a comment to explain this better.
💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295209963)
Not done yet; will do on a future push.
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295209963)
Not done yet; will do on a future push.
💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295210296)
The comment is gone (due to significant changes; the code also works differently now).
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295210296)
The comment is gone (due to significant changes; the code also works differently now).
💬 theStack commented on pull request "Update MANDATORY_SCRIPT_VERIFY_FLAGS":
(https://github.com/bitcoin/bitcoin/pull/26291#discussion_r1295210529)
Strictly speaking, wouldn't this sentence belong to `STANDARD_NOT_MANDATORY_VERIFY_FLAGS` rather than to `STANDARD_SCRIPT_VERIFY_FLAGS`? The latter includes mandatory flags, for which a violation _does_ lead to a ban/disconnect, as just stated in the same commit a few lines above ("may trigger a DoS ban").
(https://github.com/bitcoin/bitcoin/pull/26291#discussion_r1295210529)
Strictly speaking, wouldn't this sentence belong to `STANDARD_NOT_MANDATORY_VERIFY_FLAGS` rather than to `STANDARD_SCRIPT_VERIFY_FLAGS`? The latter includes mandatory flags, for which a violation _does_ lead to a ban/disconnect, as just stated in the same commit a few lines above ("may trigger a DoS ban").
💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295210753)
I haven't added a comment here as I'm not entirely sure under what scenarios `if (!node.m_sock)` triggers; it's code that existed beforehand, and is untouched by this PR.
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295210753)
I haven't added a comment here as I'm not entirely sure under what scenarios `if (!node.m_sock)` triggers; it's code that existed beforehand, and is untouched by this PR.
💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295211040)
I've added a commit that renames all receive-side functions to have "Receive" somewhere in the name.
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295211040)
I've added a commit that renames all receive-side functions to have "Receive" somewhere in the name.
💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295211509)
It can be (and is), due to optimistic send logic triggering SocketSendData, which now moves `vSendMsg` objects into the transport.
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295211509)
It can be (and is), due to optimistic send logic triggering SocketSendData, which now moves `vSendMsg` objects into the transport.
💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295214603)
It's complicated. You're right that duplicating the old `prepareForTransport` here would work, but I feel that's not really the right approach, as it's kind of hiding what's really going on. It'd also be incompatible with a potential future upgrade to using v2 transports inside the affected unit and fuzz tests.
What's really going on is that we're using the CNode's own *sending* infrastructure to construct bytes, which are then fed to the same CNode's *receive* infrastructure. But there is al
...
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1295214603)
It's complicated. You're right that duplicating the old `prepareForTransport` here would work, but I feel that's not really the right approach, as it's kind of hiding what's really going on. It'd also be incompatible with a potential future upgrade to using v2 transports inside the affected unit and fuzz tests.
What's really going on is that we're using the CNode's own *sending* infrastructure to construct bytes, which are then fed to the same CNode's *receive* infrastructure. But there is al
...
💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#issuecomment-1679726704)
I've made some significant changes to this PR:
* `DoneSendingMessage` is gone (the caller can just call `SetMessageToSend` directly, which will fail if nothing can be sent at that time).
* `HaveBytesToSend` is gone (the caller can just call `GetBytesToSend`, which will report an empty span if nothing is to be sent).
* `GetBytesToSend` now takes a `have_next_message` as input, which lets its prediction for whether there are more bytes to send afterwards be more accurate (letting it take into a
...
(https://github.com/bitcoin/bitcoin/pull/28165#issuecomment-1679726704)
I've made some significant changes to this PR:
* `DoneSendingMessage` is gone (the caller can just call `SetMessageToSend` directly, which will fail if nothing can be sent at that time).
* `HaveBytesToSend` is gone (the caller can just call `GetBytesToSend`, which will report an empty span if nothing is to be sent).
* `GetBytesToSend` now takes a `have_next_message` as input, which lets its prediction for whether there are more bytes to send afterwards be more accurate (letting it take into a
...
💬 russeree commented on issue "Raise maximum -dbcache setting":
(https://github.com/bitcoin/bitcoin/issues/28249#issuecomment-1679735177)
Would the eBPF USDT tracepoints for the UTXO cache be of use here as a way to measure the overall impact of these flush operations during and IBD?
https://github.com/bitcoin/bitcoin/tree/master/contrib/tracing#log_utxocache_flushpy
(https://github.com/bitcoin/bitcoin/issues/28249#issuecomment-1679735177)
Would the eBPF USDT tracepoints for the UTXO cache be of use here as a way to measure the overall impact of these flush operations during and IBD?
https://github.com/bitcoin/bitcoin/tree/master/contrib/tracing#log_utxocache_flushpy
💬 theStack commented on pull request "Break up script/standard.{h/cpp}":
(https://github.com/bitcoin/bitcoin/pull/28244#issuecomment-1679759413)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28244#issuecomment-1679759413)
Concept ACK
💬 ismaelsadeeq commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1295130079)
Coming from review club:
Should also be moved to secp256k1 right
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1295130079)
Coming from review club:
Should also be moved to secp256k1 right
💬 ismaelsadeeq commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1295134019)
nit:
I know you describe why the `max_size` is `1024` in commit message.
But will it be nice if you add a comment about the `1024` / `1023` `max_size`.
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1295134019)
nit:
I know you describe why the `max_size` is `1024` in commit message.
But will it be nice if you add a comment about the `1024` / `1023` `max_size`.
👍 theStack approved a pull request: "crypto: more `Span<std::byte>` modernization & follow-ups"
(https://github.com/bitcoin/bitcoin/pull/28100#pullrequestreview-1579627219)
Code-review ACK baf93fbd47058b182643c5a11fe9a8928276b05f
Potential follow-up nit: there might be some more instances where using `BOOST_CHECK_EQUAL` over `BOOST_CHECK` could make sense, see `git grep "BOOST_CHECK(.*==" ./src/test/crypto_tests.cpp`.
(https://github.com/bitcoin/bitcoin/pull/28100#pullrequestreview-1579627219)
Code-review ACK baf93fbd47058b182643c5a11fe9a8928276b05f
Potential follow-up nit: there might be some more instances where using `BOOST_CHECK_EQUAL` over `BOOST_CHECK` could make sense, see `git grep "BOOST_CHECK(.*==" ./src/test/crypto_tests.cpp`.
💬 russeree commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1679799885)
@Retropex The statement provided is fairly rife with fallacies. Noting a couple of them here with a solution at the bottom.
> Although there was a debate in particular with the BIP47, we must not forget that some people like Mike are there only to protect their business with the src-20 and stamps. They absolutely do not care about the decentralization of Bitcoin and the consequences it has on the nodes.
Ad Hominem (Attacking the Person): Just because Mike has a business interest doesn't au
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1679799885)
@Retropex The statement provided is fairly rife with fallacies. Noting a couple of them here with a solution at the bottom.
> Although there was a debate in particular with the BIP47, we must not forget that some people like Mike are there only to protect their business with the src-20 and stamps. They absolutely do not care about the decentralization of Bitcoin and the consequences it has on the nodes.
Ad Hominem (Attacking the Person): Just because Mike has a business interest doesn't au
...
💬 brunoerg commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1295294960)
Done!
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1295294960)
Done!
💬 brunoerg commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1295294996)
Done!
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1295294996)
Done!
💬 brunoerg commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1295295485)
Done!
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1295295485)
Done!
💬 brunoerg commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#issuecomment-1679841564)
Force pushed addressing https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1293959610, https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1293962271, https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1293962878 and changed the `cost_of_change` according to `spend.cpp`, it avoids any issue with low targets.
(https://github.com/bitcoin/bitcoin/pull/27585#issuecomment-1679841564)
Force pushed addressing https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1293959610, https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1293962271, https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1293962878 and changed the `cost_of_change` according to `spend.cpp`, it avoids any issue with low targets.
💬 sipa commented on issue "Raise maximum -dbcache setting":
(https://github.com/bitcoin/bitcoin/issues/28249#issuecomment-1679918327)
@Sjors When doing a full IBD from genesis with dbcache set to 16 GiB, is the limit ever reached (or gotten close to)? If not, it's probably unnecessary to raise it.
(https://github.com/bitcoin/bitcoin/issues/28249#issuecomment-1679918327)
@Sjors When doing a full IBD from genesis with dbcache set to 16 GiB, is the limit ever reached (or gotten close to)? If not, it's probably unnecessary to raise it.