💬 stringintech commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2203431643)
Could we check and return zero if `transaction_undo_index` is out of bounds here as well? (and then update the documentation in the header file that zero return value is indicative of out of bounds error)
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2203431643)
Could we check and return zero if `transaction_undo_index` is out of bounds here as well? (and then update the documentation in the header file that zero return value is indicative of out of bounds error)
💬 stringintech commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2203432158)
Might be good to include that returning zero could also be indicative of an error.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2203432158)
Might be good to include that returning zero could also be indicative of an error.
💬 stringintech commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2203435973)
In the corresponding [commit](https://github.com/bitcoin/bitcoin/commit/d031c5871e47112a93b25a0612123a5b3eaf2a33) description it is included that _the returned `kernel_TransactionOutput` is entirely owned by the user
and ..._ . It would be nice to also include the explanation here.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2203435973)
In the corresponding [commit](https://github.com/bitcoin/bitcoin/commit/d031c5871e47112a93b25a0612123a5b3eaf2a33) description it is included that _the returned `kernel_TransactionOutput` is entirely owned by the user
and ..._ . It would be nice to also include the explanation here.
💬 stringintech commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2203475130)
nit
```suggestion
* @return The block index of the current tip, or null if no active chain exists.
```
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2203475130)
nit
```suggestion
* @return The block index of the current tip, or null if no active chain exists.
```
💬 stringintech commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2203460324)
I was thinking why this is not named `kernel_set_log_level_category` as it **sets** the log level for a category. But perhaps we cannot since the name is already taken in `logging.h` for a different purpose.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2203460324)
I was thinking why this is not named `kernel_set_log_level_category` as it **sets** the log level for a category. But perhaps we cannot since the name is already taken in `logging.h` for a different purpose.
💬 stringintech commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2203462548)
Might be worth making the doc a bit more explicit to say sth like "switch back to buffering logs if no connections remain" since `DisconnectTestLogger()` doesn't clearly indicate this behavior from its name.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2203462548)
Might be worth making the doc a bit more explicit to say sth like "switch back to buffering logs if no connections remain" since `DisconnectTestLogger()` doesn't clearly indicate this behavior from its name.
💬 stringintech commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2203438688)
I think the comment does not apply here.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2203438688)
I think the comment does not apply here.
💬 stringintech commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2203480783)
As far as I understand from the implementation, there is no case we return null on error for this. Also for `kernel_copy_block_data` and `kernel_copy_block_pointer_data` functions.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2203480783)
As far as I understand from the implementation, there is no case we return null on error for this. Also for `kernel_copy_block_data` and `kernel_copy_block_pointer_data` functions.
💬 stringintech commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2203480066)
Null reason could be more explicit:
```suggestion
* @return The next block index in the currently active chain, or null if block is tip of chain.
```
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2203480066)
Null reason could be more explicit:
```suggestion
* @return The next block index in the currently active chain, or null if block is tip of chain.
```
💬 caesrcd commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3067247962)
> `DEFAULT_INCREMENTAL_RELAY_FEE` should be decreased along with this as well.
Assuming there are miners using Bitcoin Core to mine, then the `DEFAULT_BLOCK_MIN_TX_FEE` should be lowered as well.
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3067247962)
> `DEFAULT_INCREMENTAL_RELAY_FEE` should be decreased along with this as well.
Assuming there are miners using Bitcoin Core to mine, then the `DEFAULT_BLOCK_MIN_TX_FEE` should be lowered as well.
🤔 l0rinc reviewed a pull request: "[IBD] multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-3014257439)
Thanks for the review @ryanofsky!
I have addressed the comments, updated the godbolt reproducer to https://godbolt.org/z/59EMv7h6Y, and reran the big-endian tests.
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-3014257439)
Thanks for the review @ryanofsky!
I have addressed the comments, updated the godbolt reproducer to https://godbolt.org/z/59EMv7h6Y, and reran the big-endian tests.
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2203472802)
> In commit "refactor: ...
based on the code I have instead applied it to "optimization: migrate fixed-size obfuscation from `std::vector<std::byte>` to `uint64_t`" (342bb224bb96b09d9950a144e62e60ede8710191)
> target.size() > KEY_SIZE
Local benchmarks indicate that would be slower, so I kept the `target.size() >= KEY_SIZE`.
> XorWord
taken
> target.subspan(0, KEY_SIZE)
taken as `target.first<KEY_SIZE>()` and in last commit as `target.first(alignment)`
Since the benchmarks
...
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2203472802)
> In commit "refactor: ...
based on the code I have instead applied it to "optimization: migrate fixed-size obfuscation from `std::vector<std::byte>` to `uint64_t`" (342bb224bb96b09d9950a144e62e60ede8710191)
> target.size() > KEY_SIZE
Local benchmarks indicate that would be slower, so I kept the `target.size() >= KEY_SIZE`.
> XorWord
taken
> target.subspan(0, KEY_SIZE)
taken as `target.first<KEY_SIZE>()` and in last commit as `target.first(alignment)`
Since the benchmarks
...
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2203472115)
Not sure it's as dangerous as how ugly the `friend` alternative is, but I have extracted it, let me know if you see a nicer alternative.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2203472115)
Not sure it's as dangerous as how ugly the `friend` alternative is, but I have extracted it, let me know if you see a nicer alternative.
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2203471106)
Thanks
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2203471106)
Thanks
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2203471130)
Thanks, took the patch, kept `KEY_SIZE` as well. To align it with the `obfuscation_private` friend, I have extracted it outside the class.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2203471130)
Thanks, took the patch, kept `KEY_SIZE` as well. To align it with the `obfuscation_private` friend, I have extracted it outside the class.
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2203471051)
I'm not sure it matters, but I have changed it to an `int` and retested it on the emulated big-endian machine, as described above.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2203471051)
I'm not sure it matters, but I have changed it to an `int` and retested it on the emulated big-endian machine, as described above.
💬 sipa commented on pull request "cluster mempool: add TxGraph work controls":
(https://github.com/bitcoin/bitcoin/pull/32263#discussion_r2203503448)
Done. I've factored out the logic to count the number of iterations required for optimal linearization, and then used that in the txgraph simulation when `DoWork()` returns `false`.
It turned out that the implemented logic so far made this actually very hard to test for. If the remaining iterations budget in `DoWork` dropped below `m_acceptable_iterations`, but there were `NEEDS_RELINEARIZE` clusters left, the function would return `false` immediately. This would happen even when the remainin
...
(https://github.com/bitcoin/bitcoin/pull/32263#discussion_r2203503448)
Done. I've factored out the logic to count the number of iterations required for optimal linearization, and then used that in the txgraph simulation when `DoWork()` returns `false`.
It turned out that the implemented logic so far made this actually very hard to test for. If the remaining iterations budget in `DoWork` dropped below `m_acceptable_iterations`, but there were `NEEDS_RELINEARIZE` clusters left, the function would return `false` immediately. This would happen even when the remainin
...
💬 sipa commented on pull request "cluster mempool: add TxGraph work controls":
(https://github.com/bitcoin/bitcoin/pull/32263#discussion_r2203503490)
Indeed. Added some `Assume`s to that effect.
(https://github.com/bitcoin/bitcoin/pull/32263#discussion_r2203503490)
Indeed. Added some `Assume`s to that effect.
💬 supertestnet commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3067266592)
> the USD price of BTC has risen by roughly 2-3 orders of magnitude...[let's do] a 10× reduction
Wouldn't a 100x reduction be *more* in line? i.e. something like 10 sat/kvB?
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3067266592)
> the USD price of BTC has risen by roughly 2-3 orders of magnitude...[let's do] a 10× reduction
Wouldn't a 100x reduction be *more* in line? i.e. something like 10 sat/kvB?
💬 User087 commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3067277642)
To be clear, in what ways does the proposed feature resemble/differ from the [`--tx-proxy` option in monero](https://docs.getmonero.org/interacting/monerod-reference/#tori2p-and-proxies):
> Send out your local transactions through SOCKS5 proxy (Tor or I2P).
> ...
> This was introduced to make publishing transactions over Tor easier (no need for torsocks) while allowing clearnet for blocks at the same time (while torsocks affected everything).
> ...
> Note that forwarded transactions (thos
...
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3067277642)
To be clear, in what ways does the proposed feature resemble/differ from the [`--tx-proxy` option in monero](https://docs.getmonero.org/interacting/monerod-reference/#tori2p-and-proxies):
> Send out your local transactions through SOCKS5 proxy (Tor or I2P).
> ...
> This was introduced to make publishing transactions over Tor easier (no need for torsocks) while allowing clearnet for blocks at the same time (while torsocks affected everything).
> ...
> Note that forwarded transactions (thos
...